-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
nsexec: Check for errors in write_log() #3712
Conversation
First, check if strdup() fails and error out. While we are there, the else case was missing brackets, as we only need to check ret in the else case. Fix that too Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
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 comes from commit 64bb59f. The fix makes sense, so LGTM.
@cyphar PTAL The alternative would be something like this: @@ -151,7 +152,8 @@ int setns(int fd, int nstype)
static void write_log(int level, const char *format, ...)
{
- char *message = NULL, *stage = NULL, *json = NULL;
+ char *message = NULL, *json = NULL;
+ char stage[] = "nsexec\0 ";
va_list args;
int ret;
@@ -168,13 +170,16 @@ static void write_log(int level, const char *format, ...)
message = escape_json_string(message);
- if (current_stage == STAGE_SETUP)
- stage = strdup("nsexec");
- else
- ret = asprintf(&stage, "nsexec-%d", current_stage);
- if (ret < 0) {
- stage = NULL;
- goto out;
+ if (current_stage != STAGE_SETUP) {
+ char s = '?';
+ switch (current_stage) {
+ case STAGE_PARENT:
+ case STAGE_CHILD:
+ case STAGE_INIT:
+ s = '0' + current_stage;
+ }
+ stage[sizeof(stage) - 3] = '-';
+ stage[sizeof(stage) - 2] = s;
}
ret = asprintf(&json, "{\"level\":\"%s\", \"msg\": \"%s[%d]: %s\"}\n",
@@ -191,22 +196,20 @@ static void write_log(int level, const char *format, ...)
out:
free(message);
- free(stage);
free(json);
}
/* XXX: This is ugly. */
static int syncfd = -1;
(and I don't like it as it's kind of long) |
@kolyshkin I agree, I don't like that either. This PR seems more readable IMHO. Once this is merged, do we want to backport it? I have not triggered it, but I haven't run runc in mem constrained envs, so strdup can fail. I think it is safe to backport and we can avoid problems (pointer is uninitialized pointing who knows where, better to backport it and not think about it IMHO). |
Thanks! Let me know if I should create the backport PR or how that should work |
Hmm, I don't see where do we have an uninitialized pointer. When Adding curly braces remove the redundant Perhaps I am missing something here (trying to cut on my espresso intake)? |
@kolyshkin Oh, I didn't know passing NULL to %s in printf works fine. If we only support glibc, it might be ok, then. The other case, the code that was outside the else clause is executed anyways, so even without this patch if asnprintf fails, stage will be set to NULL. I was thinking too fast. I'd prefer to backport it, just in case some other lib is used or some version doesn't behave correctly, because it is a very simple patch to backport and we need to do other backports anyways. But as you and others prefer :) |
You are right that this is undefined behavior and thus needs to be fixed. Practically, though, the function we're using here (asprintf) is a gnu glibc extension, and glibc prints a %s NULL pointer as I checked that this is also implemented in musl, and they do the same thing (since BlankOn/musl@5f81468). The only issue I know is |
@rata anyway, please do open a backport against the |
@kolyshkin done! |
First, check if strdup() fails and error out.
While we are there, the else case was missing brackets, as we only need to check ret in the else case. Fix that too
Signed-off-by: Rodrigo Campos rodrigoca@microsoft.com