nsexec: Check for errors in write_log()

[rata]

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 [email protected]

[kolyshkin]

This comes from commit 64bb59f. The fix makes sense, so LGTM.

[kolyshkin]

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

[rata]

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

[rata]

Thanks! Let me know if I should create the backport PR or how that should work

[kolyshkin]

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

Hmm, I don't see where do we have an uninitialized pointer. When strdup fails, the pointer is NULL. Theoretically, it could be a problem for printf-style functions. Practically, in glibc it's OK to pass NULL as %s positional argument.

Adding curly braces remove the redundant ret check but with the current code it's just that -- a redundant check.

Perhaps I am missing something here (trying to cut on my espresso intake)?

[rata]

@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 :)

[kolyshkin]

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 (null).

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 puts(NULL) segfaults in both glibc and musl.

[kolyshkin]

@rata anyway, please do open a backport against the release-1.1 branch, it won't hurt.

[rata]

@kolyshkin done!