-
Notifications
You must be signed in to change notification settings - Fork 13.7k
fix(lib-std-fs): handle usize overflow in read*
#143462
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
Conversation
This comment has been minimized.
This comment has been minimized.
Shouldn't this fail with ErrorKind::OutOfMemory directly instead?
Also modifications to fs::read_to_string should be made to fs::read too, as they are essentially the the function.
c72c762 to
c03b767
Compare
usize overflow in read_to_string (削除ここまで)usize overflow in read* (追記ここまで)
c03b767 to
a320810
Compare
Hi, is there any particular reason you have not added regression tests for this fix? Regression tests are useful for many reasons, for example:
- Makes it easier to understand the use case
- Makes it easier to evaluate the impact of existing code
- Makes it easier to do code review
- Avoid that the fix accidentally disappears in the future
Thanks for the reminder! I do agree that tests would be a good idea. I'll have to re-read the contrib-docs, because IDK how to do it properly
I don't think a test here is necessary. The previous behaviour wasn't a bug, just a suboptimal implementation. And you'd probably have to track the amount of read syscalls in order to test this, which is way to overkill for such a simple fix.
@bors r+
r? joboet
Rollup of 5 pull requests Successful merges: - #143462 (fix(lib-std-fs): handle `usize` overflow in `read*`) - #144651 (Implementation: `#[feature(nonpoison_condvar)]`) - #145465 (Stabilize `array_repeat` feature) - #145776 (Optimize `.ilog({2,10})` to `.ilog{2,10}()`) - #145969 (Add Duration::from_nanos_u128) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #143462 - Rudxain:read_to_string_usize, r=joboet fix(lib-std-fs): handle `usize` overflow in `read*` I assume this is a non-breaking change, as there would be an OOM `panic` anyways. This patch ensures a fast-fail when there's not enough memory to load the file. This only changes behavior on platforms where `usize` is smaller than 64bits
Uh oh!
There was an error while loading. Please reload this page.
I assume this is a non-breaking change, as there would be an OOM
panicanyways. This patch ensures a fast-fail when there's not enough memory to load the file. This only changes behavior on platforms whereusizeis smaller than 64bits