Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stacktrace.cpp: Fix snprintf() usage #3916

Merged
merged 3 commits into from
Aug 3, 2023

Conversation

StephanTLavavej
Copy link
Member

C23 WP N3096 7.23.6.5 "The snprintf function"/2:

The snprintf function is equivalent to fprintf, except that the output is written into an array (specified by argument s) rather than to a stream. If n is zero, nothing is written, and s may be a null pointer. Otherwise, output characters beyond the n-1st are discarded rather than being written to the array, and a null character is written at the end of the characters actually written into the array.

In English, n should be the buffer size, INCLUDING room for a null terminator.

stacktrace.cpp consistently said sizeof("meow") - 1, which excludes the null terminator. This was a mistake.

So far I haven't seen actual misbehavior caused by this.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Aug 1, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner August 1, 2023 01:24
Copy link
Contributor

@AlexGuteniev AlexGuteniev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These constants are shared as snprintf parameter, and resize_and_overwrite parameter. The later appears not to include \0

Copy link
Contributor

@AlexGuteniev AlexGuteniev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually there is nothing wrong with the current snprintf usage.
When null terminator does not fit, snprintf still fills other characters, and return the same result.
The null terminator will be set anyway by string::resize_and_overwrite

@CaseyCarter CaseyCarter self-requested a review August 1, 2023 18:18
@CaseyCarter
Copy link
Member

Actually there is nothing wrong with the current snprintf usage. When null terminator does not fit, snprintf still fills other characters, and return the same result. The null terminator will be set anyway by string::resize_and_overwrite

snprintf always null-terminates. So when we ask snprintf to format an 8-character string into an 8-character array, we get back seven characters and a null terminator, and it returns 8. The resulting string has an "internal" \0 where we actually wanted the last character of our output to be.

TLDR: If we want to use snprintf for formatting, we have to either allocate one extra byte of space or we have to assume that the external code allocates that extra byte for us and increase the value we pass to snprintf by one. The latter is guaranteed safe by [string.capacity]/7.3 which says that [p, p + n] is a valid range (note the fully-closed range) when resize_and_overwrite calls op(p, n).

stl/src/stacktrace.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member Author

After thinking about this, Casey's suggested pattern appears to be best. I thought about avoiding the - 1 followed by the + 1 but that's really just embedding the implicit count of the null terminator so it doesn't really simplify things.

@StephanTLavavej StephanTLavavej self-assigned this Aug 3, 2023
@StephanTLavavej
Copy link
Member Author

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit e205e79 into microsoft:main Aug 3, 2023
35 checks passed
@StephanTLavavej StephanTLavavej deleted the stacktrace-snprintf branch August 3, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants