[release/9.0] Fix length check for Convert.TryToHexString{Lower} #110228
Conversation
This file contains 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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
dotnet-issue-labeler
bot
added
the
needs-area-label
adamsitnik
added
area-System.Runtime
and removed
needs-area-label
|
Tagging subscribers to this area: @dotnet/area-system-runtime |
Open
3 tasks
jeffhandley
added
Servicing-approved
jeffhandley
approved these changes
Nov 27, 2024
GrabYourPitchforks
changed the base branch from
release/9.0-staging
to
release/9.0
GrabYourPitchforks
requested review from
akoeplinger,
maraf,
ilonatommy,
lewing,
pavelsavara,
lambdageek,
steveisok and
thaystg
as code owners
GrabYourPitchforks
changed the base branch from
release/9.0
to
release/9.0-staging
GrabYourPitchforks
changed the base branch from
release/9.0-staging
to
release/9.0
The size of destination should not be less than double source's length. Fix #109807
GrabYourPitchforks
force-pushed
the
backport/pr-109891-to-release/9.0-staging
branch
from
84a732f to
3ba2b3a
Compare
carlossanlop
approved these changes
Dec 2, 2024
There was a problem hiding this comment.
Yay, 2 files once again after the force push. I assume I can merge it now @GrabYourPitchforks ?
Edit: Let's wait for the CI to finish, there's still time.
|
Rebased the current PR on release/9.0 per discussion with servicing team. |
GrabYourPitchforks
approved these changes
Dec 2, 2024
|
@carlossanlop Let's wait for CI to finish first. (Or at least to get most of the way through so we have good confidence we didn't break anything.) |
GrabYourPitchforks
removed request for
lewing,
pavelsavara,
steveisok,
lambdageek,
akoeplinger,
thaystg,
maraf and
ilonatommy
GrabYourPitchforks
changed the title
This was referenced Dec 3, 2024
|
@carlossanlop CI failures are unrelated. Feel free to merge! |
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.
Backport of #109891 to release/9.0-staging
/cc @adamsitnik @universorum
Customer Impact
The customers can't use the new
Convert.TryToHexString{Lower}methods that got added in .NET 9, as due to an invalid condition (>swapped with<), we were rejecting buffers that were large enough to be able to store the results.Regression
No, both overloads got added in .NET 9.
Testing
The issue was missed because the overload seemed simple enough to not require any new tests.
The fix contains a nice set of unit tests that exercise both happy and unhappy code paths.
Risk
I can't see any risks, it's a very, very simple fix.