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

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants