This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Fix Number.ParseNumber to not assume '\0' at the end of a span #17808
Conversation
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
This routine was written for parsing strings, which are implicitly null-terminated, and it doesn't factor in string length but instead uses tricks to exit loops when the next character is null. Now that the routine is also used for spans, this is very problematic, as spans need not be null terminated, and generally aren't when they represent slices, and expecting a null termination like this can result in walking off the end of valid memory. I would like to see all of this code rewritten to use span. In the interim, though, as a short-term fix I've changed all dereferences of the current position to compare against the length of the span (or, rather, a pointer to the end), and pretend that a null terminator was found if we've hit the end.
jkotas
approved these changes
Apr 27, 2018
danmoseley
reviewed
Apr 27, 2018
| while (true) | ||
| { | ||
| char cp = p < pEnd ? *p : '\0'; | ||
| if (cp != *str && (*str != '\u00a0' || cp != '\u0020')) |
Member
There was a problem hiding this comment.
This is a case where I think !(*str == '\u00a0' && cp == '\u0020') would be much clearer.
Member
|
In general, it is hard to prove the code in this file is safe by reading it. For example NumberBuffer is assumed to be null terminated above. And it seems it always is, but this is from several frames up, where the |
Member
Author
Yes, hence my comment about wanting to see all of this rewritten. But that's too big a change to make right now. |
stephentoub
added a commit
to stephentoub/coreclr
that referenced
this pull request
Apr 27, 2018
…t#17808) * Fix Number.ParseNumber to not assume '\0' at the end of a span This routine was written for parsing strings, which are implicitly null-terminated, and it doesn't factor in string length but instead uses tricks to exit loops when the next character is null. Now that the routine is also used for spans, this is very problematic, as spans need not be null terminated, and generally aren't when they represent slices, and expecting a null termination like this can result in walking off the end of valid memory. I would like to see all of this code rewritten to use span. In the interim, though, as a short-term fix I've changed all dereferences of the current position to compare against the length of the span (or, rather, a pointer to the end), and pretend that a null terminator was found if we've hit the end. * Address PR feedback
stephentoub
added a commit
that referenced
this pull request
Apr 30, 2018
… (#17820) * Fix Number.ParseNumber to not assume '\0' at the end of a span This routine was written for parsing strings, which are implicitly null-terminated, and it doesn't factor in string length but instead uses tricks to exit loops when the next character is null. Now that the routine is also used for spans, this is very problematic, as spans need not be null terminated, and generally aren't when they represent slices, and expecting a null termination like this can result in walking off the end of valid memory. I would like to see all of this code rewritten to use span. In the interim, though, as a short-term fix I've changed all dereferences of the current position to compare against the length of the span (or, rather, a pointer to the end), and pretend that a null terminator was found if we've hit the end. * Address PR feedback
Member
@stephentoub do we need an issue to track this? I can't find one. |
Member
Author
|
I don't believe we have one. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
This routine was written for parsing strings, which are implicitly null-terminated, and it doesn't factor in string length but instead uses tricks to exit loops when the next character is null. Now that the routine is also used for spans, this is very problematic, as spans need not be null terminated, and generally aren't when they represent slices, and expecting a null termination like this can result in walking off the end of valid memory.
I would like to see all of this code rewritten to use span. In the interim, though, as a short-term fix I've changed all dereferences of the current position to compare against the length of the span (or, rather, a pointer to the end), and pretend that a null terminator was found if we've hit the end.
Contributes to https://github.com/dotnet/corefx/issues/29343
cc: @jkotas, @danmosemsft