VPAAMP-872: Replace alloca with fixed stack arrays in logging functions - #1790
Merged
Conversation
Security scanners flag alloca() usage within loops as a potential vulnerability. While the current usage is safe (fixed 2-iteration loop with bounded sizes), replacing with fixed stack arrays eliminates the security concern with zero performance impact. Changes: - aamplogging.cpp: Replace alloca with 512-byte stack buffer for format string - PlayerLogManager.cpp: Replace alloca with 512-byte stack buffer for format string - Add bounds checking to prevent buffer overflow if format string exceeds 512 bytes Performance: No measurable impact - stack allocation occurs at function entry regardless of alloca vs fixed array. 512 bytes is sufficient for all log prefixes (timestamp + player ID + log level + thread ID + function + line). Security: Eliminates dynamic stack allocation pattern flagged by security scans.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes alloca() usage in the two logprintf() implementations by switching to a fixed-size stack buffer for the composed format string, aiming to satisfy security scanning guidance while keeping logging behavior and performance largely unchanged.
Changes:
- Replaced
alloca()-allocated format string buffers with a fixedchar format_buffer[512]stack buffer. - Added size capping logic intended to prevent writes beyond the fixed buffer when constructing the composed format string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
aamplogging.cpp |
Replace alloca() with a fixed stack buffer for composed log format string and add bounds capping. |
middleware/playerLogManager/PlayerLogManager.cpp |
Same alloca() replacement and bounds capping for middleware logging. |
Comment on lines
149
to
+153
| format_bytes++; // include nul terminator | ||
| format_ptr = (char *)alloca(format_bytes); // allocate on stack | ||
| if (format_bytes > (int)sizeof(format_buffer)) | ||
| { | ||
| format_bytes = sizeof(format_buffer); | ||
| } |
Comment on lines
127
to
+131
| format_bytes++; // include nul terminator | ||
| format_ptr = (char *)alloca(format_bytes); // allocate on stack | ||
| if (format_bytes > (int)sizeof(format_buffer)) | ||
| { | ||
| format_bytes = sizeof(format_buffer); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security scanners flag alloca() usage within loops as a potential vulnerability. While the current usage is safe (fixed 2-iteration loop with bounded sizes), replacing with fixed stack arrays eliminates the security concern with zero performance impact.
Changes:
Performance: No measurable impact - stack allocation occurs at function entry regardless of alloca vs fixed array. 512 bytes is sufficient for all log prefixes (timestamp + player ID + log level + thread ID + function + line).
Security: Eliminates dynamic stack allocation pattern flagged by security scans.