Skip to content

Overflow in libstd/io/cursor.rs "seek" returns incorrect error with large positions #39631

New issue

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

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

Already on GitHub? # to your account

Closed
amosonn opened this issue Feb 7, 2017 · 2 comments · Fixed by #39874
Closed

Comments

@amosonn
Copy link
Contributor

amosonn commented Feb 7, 2017

Seems to be a reroll #20678. I have some idea how to fix it, will try to draft a pull request later, but it basically requires checking the sign of the i64, negging if required and casting to u64, and using checked_add or checked_sub.

I will divide into cases to demonstrate that this is required, to save repeating phrases, I will use the following notation:
S means pos < i64::max_value(), L means pos > i64::max_value();
P means n >= 0, N means n < 0;
O+ means we will notice bads by checking OF on add (u64::checked_add), O- means we will notice them by checking OF on sub (u64::checked_sub after i64::wrapping_neg), Z+ means we will notice by adding as i64 and making sure result is non-negative, Z- is making sure it is negative, D+ mean we will notice bads by asserting OF is set on add (u64::overflowing_add), D- is likewise for sub (u64::overflowing_sub). Note the we don't need to consider n == i64::min_value(), because after i64::wrapping_neg and as u64 we still get the right, large, positive number.
So, we have the following cases and appropriate checks that spot trouble:

  • SP -> O+Z+D- (there's never a problem, but the others false-positive)
  • LP -> O+Z-D- (but D- requires also that the result is non-zero).
  • SN -> O-Z+D+
  • LN -> O-D+ (there's never a problem, but the others false-positive)

Overall, we see that there is no one test which works for all cases, and a minimum of two cases (and tests) yields the solution described above.

@alexcrichton
Copy link
Member

Thanks for the report! I'd definitely believe that the exact implementation here is slightly wrong and could use some tweaking.

@amosonn
Copy link
Contributor Author

amosonn commented Feb 16, 2017 via email

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 16, 2017
Fixes overflow in libsdt/io/cursor.rs "seek"

Fixes rust-lang#39631
Test which fails (with old implementation), then fix to implementation.
frewsxcv added a commit to frewsxcv/rust that referenced this issue Feb 17, 2017
Fixes overflow in libsdt/io/cursor.rs "seek"

Fixes rust-lang#39631
Test which fails (with old implementation), then fix to implementation.
frewsxcv added a commit to frewsxcv/rust that referenced this issue Feb 17, 2017
Fixes overflow in libsdt/io/cursor.rs "seek"

Fixes rust-lang#39631
Test which fails (with old implementation), then fix to implementation.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 17, 2017
Fixes overflow in libsdt/io/cursor.rs "seek"

Fixes rust-lang#39631
Test which fails (with old implementation), then fix to implementation.
bors added a commit that referenced this issue Feb 21, 2017
Fixes overflow in libsdt/io/cursor.rs "seek"

Fixes #39631
Test which fails (with old implementation), then fix to implementation.
@amosonn amosonn changed the title Overflow in libsdt/io/cursor.rs "seek" returns incorrect error with large positions Overflow in libstd/io/cursor.rs "seek" returns incorrect error with large positions Feb 10, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants