-
Notifications
You must be signed in to change notification settings - Fork 239
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
utils: Don't let ssize_t overflow when reading very large files #624
Conversation
The size to be allocated is tracked as ssize_t, so if it's larger than this, doubling it would cause a signed overflow. Limiting the data we will read into memory to SSIZE_MAX/2 still lets it occupy 25% of addressable memory (1 GiB on 32-bit or some very large amount on 64-bit), which should be adequate. In practice we expect this function to read a few KiB at most. In practice we're likely to run out of memory before reaching this point; changing this to SSIZE_MAX / 8, compiling as 32-bit and running `${builddir}/bwrap --args 0 < /dev/zero` is a convenient way to test this code path. Fixes: 422c078 "Check for allocation size overflows" Signed-off-by: Simon McVittie <smcv@collabora.com>
@cgwalters @alexlarsson: any chance of a review on this? I would really prefer to avoid bubblewrap getting into a situation where my changes can't get reviewed because I'm the only active reviewer. In general, I would like some help from other maintainers of this project. I'm doing my best to keep it afloat, but my best is not enough: I hope it's already become obvious that having me as the only one doing releases doesn't scale (see also: dbus), and I just don't have the in-depth syscall-level knowledge to assess whether some of the other PRs would be creating security vulnerabilities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition is indeed wrong.
I'm going to merge this based on @cgzones' non-maintainer review. Please revert if you disagree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks right. That said, there is now increasing usage of __builtin_mul_overflow
in C projects (e.g. Linux wraps it as check_mul_overflow
) for similar situations.
I'm still around and yeah, it's OK to ping for things like this. Thanks so much for your work! |
* `--symlink` is now idempotent, meaning it succeeds if the symlink already exists and already has the desired target (containers/bubblewrap#549, flatpak#2387, flatpak#3477, flatpak#5255) * Report a better error message if `mount(2)` fails with `ENOSPC` (containers/bubblewrap#615, ValveSoftware/steam-runtime#637) * Fix a double-close on error reading from `--args`, `--seccomp` or `--add-seccomp-fd` argument (containers/bubblewrap#558) * Improve memory allocation behaviour (containers/bubblewrap#556, containers/bubblewrap#624) * Silence various compiler warnings (containers/bubblewrap#559) Resolves: flatpak#2387 Resolves: flatpak#3477 Resolves: flatpak#5255 Signed-off-by: Simon McVittie <smcv@collabora.com>
* `--symlink` is now idempotent, meaning it succeeds if the symlink already exists and already has the desired target (containers/bubblewrap#549, #2387, #3477, #5255) * Report a better error message if `mount(2)` fails with `ENOSPC` (containers/bubblewrap#615, ValveSoftware/steam-runtime#637) * Fix a double-close on error reading from `--args`, `--seccomp` or `--add-seccomp-fd` argument (containers/bubblewrap#558) * Improve memory allocation behaviour (containers/bubblewrap#556, containers/bubblewrap#624) * Silence various compiler warnings (containers/bubblewrap#559) Resolves: #2387 Resolves: #3477 Resolves: #5255 Signed-off-by: Simon McVittie <smcv@collabora.com>
* `--symlink` is now idempotent, meaning it succeeds if the symlink already exists and already has the desired target (containers/bubblewrap#549, flatpak#2387, flatpak#3477, flatpak#5255) * Report a better error message if `mount(2)` fails with `ENOSPC` (containers/bubblewrap#615, ValveSoftware/steam-runtime#637) * Fix a double-close on error reading from `--args`, `--seccomp` or `--add-seccomp-fd` argument (containers/bubblewrap#558) * Improve memory allocation behaviour (containers/bubblewrap#556, containers/bubblewrap#624) * Silence various compiler warnings (containers/bubblewrap#559) Resolves: flatpak#2387 Resolves: flatpak#3477 Resolves: flatpak#5255 Signed-off-by: Simon McVittie <smcv@collabora.com>
The size to be allocated is tracked as ssize_t, so if it's larger than this, doubling it would cause a signed overflow.
Limiting the data we will read into memory to SSIZE_MAX/2 still lets it occupy 25% of addressable memory (1 GiB on 32-bit or some very large amount on 64-bit), which should be adequate. In practice we expect this function to read a few KiB at most.
In practice we're likely to run out of memory before reaching this point; changing this to SSIZE_MAX / 8, compiling as 32-bit and running
${builddir}/bwrap --args 0 < /dev/zero
is a convenient way to test this code path.Fixes: 422c078 "Check for allocation size overflows"
/cc @cgzones