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

Don't memcmp(x, y, -1) from ranges::lexicographical_compare #3989

Merged
merged 2 commits into from
Aug 31, 2023

Conversation

CaseyCarter
Copy link
Member

@CaseyCarter CaseyCarter commented Aug 23, 2023

... even with two potentially-infinite ranges.

Drive by: Pull non-dependent test cases out of the instantiator::call template magic in tests/P0896R4_ranges_alg_lexicographical_compare.

Fixes VSO-1854238 / AB#1854238

ASan considers calls to `memcmp` with size `-1` to indicate bugs. These two test cases are the exception that proves the rule.

Drive by: Pull non-dependent test cases out of the `instantiator::call` template magic.

Fixes VSO-1854238 / AB#1854238
@CaseyCarter CaseyCarter added test Related to test code ASan Address Sanitizer labels Aug 23, 2023
@CaseyCarter CaseyCarter requested a review from a team as a code owner August 23, 2023 20:47
@strega-nil-ms
Copy link
Contributor

I think we should maybe reconsider calling memcmp(x, y, -1) in this case entirely, at least under __SANITIZE_ADDRESS__, since, while memcmp is UB when you don't call it with fully valid buffers1, calling lexicographical_compare(i1, unreachable_sentinel, i2, unreachable_sentinel) is valid (at least, that's my understanding; I may fully be wrong about that and this is a test of an additional guarantee we provide).

Footnotes

  1. https://trust-in-soft.com/blog/2015/12/21/memcmp-requires-pointers-to-fully-valid-buffers/

@CaseyCarter
Copy link
Member Author

I think we should maybe reconsider calling memcmp(x, y, -1) in this case entirely, at least under __SANITIZE_ADDRESS__, since, while memcmp is UB when you don't call it with fully valid buffers1, calling lexicographical_compare(i1, unreachable_sentinel, i2, unreachable_sentinel) is valid (at least, that's my understanding; I may fully be wrong about that and this is a test of an additional guarantee we provide).

Footnotes

  1. https://trust-in-soft.com/blog/2015/12/21/memcmp-requires-pointers-to-fully-valid-buffers/

Yeah, it's hard to argue in support of memcmp(x, y, -1). The initial motivation was "it works, and doing so means we don't need a special-case check for the case where both ranges are 'infinite'". But given how questionable this is in the first place and how quickly we've found a case where it doesn't "work", I should just add the special-case check and be done with it.

@CaseyCarter CaseyCarter changed the title Avoid memcmp(x, y, -1) with ASan Avoid memcmp(x, y, -1) ~~with ASan~~ Aug 25, 2023
@CaseyCarter CaseyCarter changed the title Avoid memcmp(x, y, -1) ~~with ASan~~ Don't memcmp(x, y, -1) from ranges::lexicographical_compare Aug 25, 2023
@CaseyCarter
Copy link
Member Author

Okay, fixed. Thanks for reminding me how icky this was in the first place.

@CaseyCarter CaseyCarter self-assigned this Aug 29, 2023
@CaseyCarter
Copy link
Member Author

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

@CaseyCarter CaseyCarter merged commit 46ca2d7 into microsoft:main Aug 31, 2023
@CaseyCarter CaseyCarter deleted the ranges_non_bug branch August 31, 2023 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ASan Address Sanitizer test Related to test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants