-
Notifications
You must be signed in to change notification settings - Fork 235
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
Allocation improvements #556
Conversation
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 xcalloc()
changes all look great. For the realloc()
change, I'll need to take a closer look to make sure we don't have any callers that could pass in (nonnull, 0)
.
Pass the first parameter to calloc(3) to perform the overflow check. Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Kindly Ping. |
utils.c
Outdated
|
||
if (size != 0 && res == NULL) | ||
assert (ptr == NULL || size != 0); |
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.
This implements "crash with an assertion failure if called with (NULL, 0)", but that's not really the same thing as "check the rest of the source code to make sure nobody calls it with (NULL, 0) at the moment". Did you also do the latter?
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.
If it would be easier to check that size is never 0 for any caller, then doing that instead would also be fine (and this assertion could be simplified).
utils.c
Outdated
@@ -594,6 +599,8 @@ load_file_data (int fd, | |||
{ | |||
if (data_len == data_read + 1) | |||
{ | |||
if (data_len > SIZE_MAX / 2) | |||
return NULL; |
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.
This needs to either exit the program with an appropriate fatal error (die_oom()
seems close enough, or use die ("File too large to read into memory")
or similar), or set errno
(perhaps to ENOMEM
) before returning null. Otherwise, the function will return NULL without setting a correct error.
utils.c
Outdated
@@ -820,6 +827,8 @@ readlink_malloc (const char *pathname) | |||
|
|||
do | |||
{ | |||
if (size > SIZE_MAX / 2) | |||
return NULL; |
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.
As above, this needs to either set errno, or die with a fatal error.
Avoid integer overflows in allocation size variables to avoid passing 0 to xrealloc(). Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Passing a NULL pointer and a size of 0 is currently implementation defined behavior[1] and will become (with C23) undefined behavior[2]. [1]: https://manpages.debian.org/unstable/manpages-dev/realloc.3.en.html [2]: https://en.cppreference.com/w/c/memory/realloc Assert the passed size is always non-zero, which the current codebase satisfies. Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
For future reference, I'd have got back to this quicker if you had replied to the review threads to say you had fixed them, or marked them as resolved. |
* `--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>
Rework xcalloc
Pass the first parameter to calloc(3) to perform the overflow check.
Avoid undefined behavior in realloc
Passing a NULL pointer and a size of 0 is currently implementation defined behavior1 and will become (with C23) undefined behavior2.