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

utils: Don't let ssize_t overflow when reading very large files #624

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

smcv
Copy link
Collaborator

@smcv smcv commented Mar 25, 2024

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

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>
@smcv
Copy link
Collaborator Author

smcv commented Mar 26, 2024

@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.

Copy link
Contributor

@cgzones cgzones left a 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.

@smcv
Copy link
Collaborator Author

smcv commented Mar 26, 2024

I'm going to merge this based on @cgzones' non-maintainer review. Please revert if you disagree.

@smcv smcv merged commit 041e3c5 into containers:main Mar 26, 2024
5 checks passed
Copy link
Collaborator

@cgwalters cgwalters left a 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.

@cgwalters
Copy link
Collaborator

@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.

I'm still around and yeah, it's OK to ping for things like this. Thanks so much for your work!

smcv added a commit to smcv/flatpak that referenced this pull request Mar 27, 2024
* `--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>
smcv added a commit to flatpak/flatpak that referenced this pull request Mar 27, 2024
* `--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>
GeorgesStavracas pushed a commit to GeorgesStavracas/flatpak that referenced this pull request Apr 26, 2024
* `--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>
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 this pull request may close these issues.

3 participants