Conversation
mz-pdm
requested review from
MatiasVara,
dorindabassey,
jakecorrenti,
mtjhrc,
slp and
tylerfanelli
as code owners
|
I don't know how to get "entrypoint" entry in .krun_config.json so I couldn't test the corresponding part of the fix. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an important issue of buffer overflows and silent argument dropping by replacing the fixed-size MAX_ARGS with dynamic memory allocation based on the actual number of arguments. The changes in config_parse_args correctly implement this. However, the changes in concat_entrypoint_argv introduce a critical bug by using an uninitialized variable n_args to calculate the allocation size, which will lead to undefined behavior. This must be fixed before merging.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors argument parsing and concatenation functions in init.c by removing the fixed MAX_ARGS limit and dynamically allocating memory based on the actual number of arguments. The changes include updating config_parse_args to use token->size for allocation and concat_entrypoint_argv to calculate the total number of arguments before allocation. Review feedback highlights critical issues where the return values of malloc are not checked in both config_parse_args and concat_entrypoint_argv, which could lead to segmentation faults. Additionally, an efficiency improvement was suggested for concat_entrypoint_argv to avoid redundant iterations and use memcpy for copying arguments.
| for (i = 0; entrypoint[i]; i++) | ||
| n_args++; | ||
| for (j = 0; config_argv[j]; j++) | ||
| n_args++; | ||
|
|
||
| argv = malloc(MAX_ARGS * sizeof(char *)); | ||
| argv = malloc((n_args + 1) * sizeof(char *)); | ||
|
|
||
| for (i = 0; i < MAX_ARGS && entrypoint[i]; i++) { | ||
| for (i = 0; entrypoint[i]; i++) { | ||
| argv[i] = entrypoint[i]; | ||
| } | ||
|
|
||
| for (j = 0; j < MAX_ARGS && config_argv[j]; i++, j++) { | ||
| for (j = 0; config_argv[j]; i++, j++) { | ||
| argv[i] = config_argv[j]; | ||
| } |
There was a problem hiding this comment.
There are two issues in this function:
- Critical: The return value of
mallocon line 700 is not checked. Ifmallocfails, it will cause a segmentation fault on line 703. - Medium: The current implementation iterates over
entrypointandconfig_argvarrays twice (once for counting, once for copying). This is inefficient.
A more robust and efficient implementation would check for malloc failure and use memcpy for copying after determining the lengths of the arrays. Here is a suggested refactoring of the function body:
char **argv;
int entrypoint_len = 0;
int config_argv_len = 0;
while (entrypoint[entrypoint_len]) {
entrypoint_len++;
}
while (config_argv[config_argv_len]) {
config_argv_len++;
}
int n_args = entrypoint_len + config_argv_len;
argv = malloc((n_args + 1) * sizeof(char *));
if (!argv) {
perror("malloc");
return NULL;
}
memcpy(argv, entrypoint, entrypoint_len * sizeof(char *));
memcpy(argv + entrypoint_len, config_argv, config_argv_len * sizeof(char *));
argv[n_args] = NULL;
return argv;Note that i and j would need to be removed from the variable declarations if you use this approach.
There was a problem hiding this comment.
This may not be a bad idea but it introduces unnecessary changes to the problem being fixed and I consider the original code a bit better readable. As for malloc() return value, this is handled in the newly added commit.
There are a few malloc() calls in init.c without accompanied checks for their return values. All of them are in argv processing. Let's add the missing checks to them. In case of an allocation error, we simply print the error and return NULL, which is similar to handling memory allocation errors in other places in init.c. NULL is a valid return value in the given cases. It leads to the arguments being ignored and running the default command defined in init.c (/bin/sh). This is not completely correct but is still better than the original segmentation faults. Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
The number of the command line arguments is limited by MAX_ARGS constant in init.c. When parsing the arguments there, `argv' arrays to store pointers to them are allocated with wrong sizes: - Space for the closing NULL marker is not allocated. - In config_parse_args, there is no check on MAX_ARGS when adding the arguments to the array. - In concat_entrypoint_argv, there can be up to 2 * MAX_ARGS entries (two loops adding them, each with up to MAX_ARGS iterations) but the allocated array size is only MAX_ARGS. This patch fixes these problems. When on it, the useless `j' variable in config_parse_args that just mimics `i' value is removed. Silently ignoring the provided command line arguments is generally wrong and dangerous (imagine the last argument is e.g. --dry-run). This is addressed in the followup patch; this patch serves just for starting from a correct basis. Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
init.c imposes limit of MAX_ARGS command line arguments. If there are
more arguments then they are silently ignored. Which is wrong and
dangerous, imagine one of those arguments being e.g. --dry-run or
--safe.
There is no reason to limit the number of the arguments to 32 (the value
of MAX_ARGS) declared in init.c. Common limits on the number of
arguments are much higher, if any. The arguments are passed in the
config file provided to the VM, it's the responsibility of the caller to
provide sane arguments. Which means we can lift the limit completely
and to allocate `argv' arrays to the exact size needed rather than to
the predefined maximum limit.
For example, the following command works fine for me with this patch,
while printing only the first 31 numbers previously:
podman run --runtime krun --rm -t \
docker.io/library/alpine echo $(seq 1 10000)
Fixes: containers#323
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors argument parsing and concatenation functions (config_parse_args and concat_entrypoint_argv) in init.c. It removes the MAX_ARGS constant and replaces fixed-size array allocations with dynamic memory allocation based on the actual number of arguments. Additionally, malloc error checks and cleanup logic have been added to improve robustness. I have no feedback to provide on the current changes.
|
The CI failures look unrelated, is there a way to rerun? |
2 participants
The command line argument processing in init.c writes outside allocated memory and silently drops command line arguments when there are many. This PR fixes these problems and #323.