Undefined Behavior: write past the end of an array

alejandro-colomar commented 1 year ago

While trying to fix a bug, I found these:

$ grepc -tfd nxt_clone_credential_setgroups
./src/nxt_clone.c:37:
nxt_int_t
nxt_clone_credential_setgroups(nxt_task_t *task, pid_t child_pid,
    const char *str)
{
    int     fd, n;
    u_char  *p, *end;
    u_char  path[PATH_MAX];

    end = path + PATH_MAX;
    p = nxt_sprintf(path, end, "/proc/%d/setgroups", child_pid);
    *p = '\0';

    if (nxt_slow_path(p == end)) {
        nxt_alert(task, "error write past the buffer: %s", path);
        return NXT_ERROR;
    }

    ...
}

$ grepc -tfd nxt_discovery_modules
./src/nxt_application.c:205:
static nxt_buf_t *
nxt_discovery_modules(nxt_task_t *task, const char *path)
{
    ...

    *p++ = ']';

    if (nxt_slow_path(p > end)) {
        nxt_alert(task, "discovery write past the buffer");
        goto fail;
    }

    ...
}

$ grepc -tfd nxt_clone_credential_map_set
./src/nxt_clone.c:133:
nxt_int_t
nxt_clone_credential_map_set(nxt_task_t *task, const char* mapfile, pid_t pid,
    nxt_int_t default_container, nxt_int_t default_host,
    nxt_clone_credential_map_t *map)
{
    ...

    if (map->size > 0) {
        ...

    } else {
        ...

        end = mapinfo + len;
        p = nxt_sprintf(mapinfo, end, "%d %d 1",
                        default_container, default_host);
        *p = '\0';

        if (nxt_slow_path(p == end)) {
            nxt_alert(task, "write past mapinfo buffer");
            nxt_free(mapinfo);
            return NXT_ERROR;
        }
    }

    ...
}

Writing past the end is invoking Undefined Behavior, and checking after it has happened is too late (the compiler may even remove those checks, assuming they are impossible). We should move the checks to before the writing.

alejandro-colomar commented 1 year ago

However, that doesn't fix the other important problem with this code: we're not really detecting string truncation at all.

nxt_sprintf() needs to be fixed to be able to report truncation. A good example of how to do that, IMO, would be stpecpy(), which returns one-past-the-end on truncation: https://software.codidact.com/posts/285946

CC: @andrey-zelenkov @hongzhidao @ac000

alejandro-colomar commented 1 year ago

Suggested workaround:

diff --git a/src/nxt_application.c b/src/nxt_application.c
index 786c768b..9317930b 100644
--- a/src/nxt_application.c
+++ b/src/nxt_application.c
@@ -309,18 +309,23 @@ nxt_discovery_modules(nxt_task_t *task, const char *path)
                             mnt[j].data == NULL ? (u_char *) "" : mnt[j].data);
         }

+        if (nxt_slow_path(p > end - 3)) {
+            nxt_alert(task, "discovery buffer truncation");
+            goto fail;
+        }
+
         *p++ = ']';
         *p++ = '}';
         *p++ = ',';
     }

-    *p++ = ']';
-
-    if (nxt_slow_path(p > end)) {
-        nxt_alert(task, "discovery write past the buffer");
+    if (nxt_slow_path(p > end - 1)) {
+        nxt_alert(task, "discovery buffer truncation");
         goto fail;
     }

+    *p++ = ']';
+
     b->mem.free = p;

 fail:
diff --git a/src/nxt_clone.c b/src/nxt_clone.c
index a9b39ac1..e43e8304 100644
--- a/src/nxt_clone.c
+++ b/src/nxt_clone.c
@@ -44,13 +44,13 @@ nxt_clone_credential_setgroups(nxt_task_t *task, pid_t child_pid,

     end = path + PATH_MAX;
     p = nxt_sprintf(path, end, "/proc/%d/setgroups", child_pid);
-    *p = '\0';
-
     if (nxt_slow_path(p == end)) {
-        nxt_alert(task, "error write past the buffer: %s", path);
+        nxt_alert(task, "buffer truncation: %s", path);
         return NXT_ERROR;
     }

+    *p = '\0';
+
     fd = open((char *)path, O_RDWR);

     if (fd == -1) {
@@ -183,13 +183,13 @@ nxt_clone_credential_map_set(nxt_task_t *task, const char* mapfile, pid_t pid,
         end = mapinfo + len;
         p = nxt_sprintf(mapinfo, end, "%d %d 1",
                         default_container, default_host);
-        *p = '\0';
-
         if (nxt_slow_path(p == end)) {
-            nxt_alert(task, "write past mapinfo buffer");
+            nxt_alert(task, "truncation in mapinfo buffer");
             nxt_free(mapinfo);
             return NXT_ERROR;
         }
+
+        *p = '\0';
     }

     ret = nxt_clone_credential_map_write(task, mapfile, pid, mapinfo);

This workaround avoids overrunning the arrays.

However, this is not a complete fix. The fix should improve truncation detection in nxt_sprintf() by changing the design of the function.

alejandro-colomar commented 1 year ago

Also, copying with *p++ = 'c'; is dangerous if truncation is possible, and should be replaced by something that can avoid this issue, and simplifies truncation detection.

alejandro-colomar commented 1 year ago

Another case:

$ grepc -ctu nxt_cpystrn src/nxt_log.c 
src/nxt_log.c:66:
void nxt_cdecl
nxt_log_handler(nxt_uint_t level, nxt_log_t *log, const char *fmt, ...)
{
    u_char   *p, *end;
#if 0
    u_char   *syslogmsg;
#endif
    va_list  args;
    u_char   msg[NXT_MAX_ERROR_STR];

    p = msg;
    end = msg + NXT_MAX_ERROR_STR;

    if (nxt_log_prefix != NULL) {
        p = nxt_cpystrn(p, nxt_log_prefix, end - p);
        *p++ = ':';
        *p++ = ' ';
    }

#if 0
    syslogmsg = p;
#endif

    p = nxt_sprintf(p, end, (log->ident != 0) ? "[%V] *%D " : "[%V] ",
                    &nxt_log_levels[level], log->ident);

    va_start(args, fmt);
    p = nxt_vsprintf(p, end, fmt, args);
    va_end(args);

    if (level != NXT_LOG_DEBUG && log->ctx_handler != NULL) {
        p = log->ctx_handler(log->ctx, p, end);
    }

    if (p > end - nxt_length("\n")) {
        p = end - nxt_length("\n");
    }

    *p++ = '\n';

    (void) nxt_write_console(nxt_stderr, msg, p - msg);

#if 0
    if (level == NXT_LOG_ALERT) {
        *(p - nxt_length("\n")) = '\0';

        /*
         * Syslog LOG_ALERT level is enough, because
         * LOG_EMERG level broadcast a message to all users.
         */
        nxt_write_syslog(LOG_ALERT, syslogmsg);
    }
#endif
}

The bug is here:

$ grepc -ctu nxt_cpystrn src/nxt_log.c 
src/nxt_log.c:66:
void nxt_cdecl
nxt_log_handler(nxt_uint_t level, nxt_log_t *log, const char *fmt, ...)
{
    u_char   *p, *end;
#if 0
    u_char   *syslogmsg;
#endif
    va_list  args;
    u_char   msg[NXT_MAX_ERROR_STR];

    p = msg;
    end = msg + NXT_MAX_ERROR_STR;

    if (nxt_log_prefix != NULL) {
        p = nxt_cpystrn(p, nxt_log_prefix, end - p);
        *p++ = ':';
        *p++ = ' ';
    }

    ...
}

Good news, NXT_MAX_ERROR_STR is big, and nxt_log_prefix is small (it comes from the app name, or argv[0], AFAICS), so this is very unlikely to happen, and since the app name is only controlled by the administrator, it's not exploitable remotely.

Basically, nxt_cpystrn() copies from a (null-terminated) string with truncation, and it guarantees null-termination, returning a pointer to the terminating null byte. It truncates at NXT_MAX_ERROR_STR, so it won't overrun, BUT *p++ can overrun: the first one *p++ = ':'; will overwrite the null byte, and the second one *p++ = ' '; will write a space at an unknown memory location.

@hongzhidao

alejandro-colomar commented 1 year ago

The following function is better than nxt_cpystrn(). It is simpler to use, and detects truncation.

/* This code is in the public domain. */
char *
stpecpy(char *dst, const char *restrict src, char past_end[0])
{
    char *p;

    if (dst == past_end)
        return past_end;

    p = memccpy(dst, src, '\0', past_end - dst);
    if (p != NULL)
        return p - 1;

    /* truncation detected */
    past_end[-1] = '\0';
    return past_end;
}

You can find the original design of this function here: https://software.codidact.com/posts/285946/287522#answer-287522

It would be used as:

    p = msg;
    past_end = msg + NXT_MAX_ERROR_STR;

    if (nxt_log_prefix != NULL) {
        p = nxt_stpecpy(p, nxt_log_prefix, past_end);
        p = nxt_stpecpy(p, ": ", past_end);
    }

We could add it to nxt_string.h/c and replace the only two cases of nxt_cpystrn(). It would also be useful to replace some snprintf calls, such as the ones recently added by @ac000 .

BTW, @igorsysoev , you're invited to comment on this (and a few others) string copying function. I'm going to rewrite the Linux man-pages regarding string copying, and your input might be very useful.

hongzhidao commented 1 year ago

Hi,

If you think it's a possible issue that someone uses too long argv[0], we can fix it like this without introducing a new API.

diff -r 6e5a9550ead3 src/nxt_log.c
--- a/src/nxt_log.c     Thu Nov 17 21:56:58 2022 +0000
+++ b/src/nxt_log.c     Sun Dec 11 00:02:34 2022 +0800
@@ -77,9 +77,7 @@
 end = msg + NXT_MAX_ERROR_STR;

 if (nxt_log_prefix != NULL) {
-        p = nxt_cpystrn(p, nxt_log_prefix, end - p);
-        *p++ = ':';
-        *p++ = ' ';
+        p = nxt_sprintf(p, end, "%s: ", nxt_log_prefix);
 }

In my feelings, we don't need to fix it, we are not creating perfect oss, it's rarely used like it.

alejandro-colomar commented 1 year ago

Hi,

1. If you think it's a possible issue that someone uses too long argv[0], we can fix it like this without introducing a new API.

diff -r 6e5a9550ead3 src/nxt_log.c
--- a/src/nxt_log.c     Thu Nov 17 21:56:58 2022 +0000
+++ b/src/nxt_log.c     Sun Dec 11 00:02:34 2022 +0800
@@ -77,9 +77,7 @@
     end = msg + NXT_MAX_ERROR_STR;

     if (nxt_log_prefix != NULL) {
-        p = nxt_cpystrn(p, nxt_log_prefix, end - p);
-        *p++ = ':';
-        *p++ = ' ';
+        p = nxt_sprintf(p, end, "%s: ", nxt_log_prefix);
     }

Hmm, yes, this case can be fixed with nxt_sprintf(), because it doesn't require a terminating null byte. So, yes, this is a good fix too (except that a formatting function is unnecessary overhead, but since this is for logging, it's probably not that bad).

In the case of @ac000 's code, that solution is not valid, because nxt_sprintf() is not reliable for null-terminated strings.

In my feelings, we don't need to fix it, we are not creating perfect oss, it's rarely used like it.

hongzhidao commented 1 year ago

So,

We don't need to rework nxt_log.c.
I'm not sure if there is an issue in Andrew's code, we didn't find it before it was released. If yes, maybe it's better to create an issue for it.

alejandro-colomar commented 1 year ago

And yet one more (in this case we don't write, but we hold a pointer to invalid memory; the program might crash):

$ grepc nxt_unit_req_log
./src/nxt_unit.h:361:
void nxt_unit_req_log(nxt_unit_request_info_t *req, int level,
    const char* fmt, ...) NXT_ATTR_FORMAT;

./src/nxt_unit.c:6621:
void
nxt_unit_req_log(nxt_unit_request_info_t *req, int level, const char *fmt, ...)
{
    int                           log_fd, n;
    char                          msg[NXT_MAX_ERROR_STR], *p, *end;
    pid_t                         pid;
    va_list                       ap;
    nxt_unit_impl_t               *lib;
    nxt_unit_request_info_impl_t  *req_impl;

    if (nxt_fast_path(req != NULL)) {
        lib = nxt_container_of(req->ctx->unit, nxt_unit_impl_t, unit);

        pid = lib->pid;
        log_fd = lib->log_fd;

    } else {
        pid = nxt_unit_pid;
        log_fd = STDERR_FILENO;
    }

    p = msg;
    end = p + sizeof(msg) - 1;

    p = nxt_unit_snprint_prefix(p, end, pid, level);

    if (nxt_fast_path(req != NULL)) {
        req_impl = nxt_container_of(req, nxt_unit_request_info_impl_t, req);

        p += snprintf(p, end - p, "#%"PRIu32": ", req_impl->stream);
    }

    va_start(ap, fmt);
    p += vsnprintf(p, end - p, fmt, ap);
    va_end(ap);

    if (nxt_slow_path(p > end)) {
        memcpy(end - 5, "[...]", 5);
        p = end;
    }

    *p++ = '\n';

    n = write(log_fd, msg, p - msg);
    if (nxt_slow_path(n < 0)) {
        fprintf(stderr, "Failed to write log: %.*s", (int) (p - msg), msg);
    }
}

The bug is made up of several bugs:

void
nxt_unit_req_log(nxt_unit_request_info_t *req, int level, const char *fmt, ...)
{
    ...
    char                          msg[NXT_MAX_ERROR_STR], *p, *end;
    ...
    end = p + sizeof(msg) - 1;

    ...

    if (nxt_slow_path(p > end)) {
        memcpy(end - 5, "[...]", 5);
        p = end;
    }

    ...
}

That is undefined behavior: If p is greater than end, then the buffer has already been overrun. An exception is if p == end + 1, which is still valid.

$ grepc nxt_unit_req_log
./src/nxt_unit.h:361:
void nxt_unit_req_log(nxt_unit_request_info_t *req, int level,
    const char* fmt, ...) NXT_ATTR_FORMAT;

./src/nxt_unit.c:6621:
void
nxt_unit_req_log(nxt_unit_request_info_t *req, int level, const char *fmt, ...)
{
    ...

    p = msg;
    end = p + sizeof(msg) - 1;

    p = nxt_unit_snprint_prefix(p, end, pid, level);

    if (nxt_fast_path(req != NULL)) {
        req_impl = nxt_container_of(req, nxt_unit_request_info_impl_t, req);

        p += snprintf(p, end - p, "#%"PRIu32": ", req_impl->stream);
    }

    va_start(ap, fmt);
    p += vsnprintf(p, end - p, fmt, ap);
    va_end(ap);

    ...
}

Here we see 3 consecutive calls to snprintf(3) (nxt_unit_snprint_prefix() calls it internally). snprintf(3) returns the length of the string that would have been written if the destination buffer was infinite, and so adding its result to p results in possibly holding a pointer to invalid memory.

Again, the underlying issue is concatenating snprintf(3). That libc function has not been designed with chain calls in mind, and using it for that is unsafe. The bad part is that there's no existing function that can be used for that.

We need to add a wrapper that has similar semantics as stpecpy() (proposed in https://github.com/nginx/unit/pull/805), which I'd call stpeprintf(), which could be called as p = stpeprintf(p, past_end, "fmt", ...);

alejandro-colomar commented 1 year ago

Suggestion for a better replacement for snprintf(3):

$ cat stpeprintf.c 
char *
stpeprintf(char *str, char past_end[0], const char *restrict fmt, ...)
{
    int      len;
    char     *p;
    va_list  ap;

    if (str == past_end)
        return past_end;

    if (unlikely(str == NULL))
        return NULL;

    va_start(ap, fmt);
    len = vsnprintf(str, past_end - str, fmt, ap);
    va_end(ap);

    if (unlikely(len == -1))
        return NULL;

    if (len >= past_end - str)
        return past_end;

    return str + len;
}

This function, like stpecpy(), allows chaining and only checking for truncation at the end of the chain. Since it has the same semantics as stpecpy(), it can even be combined with stpecpy() as long as you know that the length of the strings won't go past INT_MAX (in which case this function returns NULL), which normally should be a reasonable assumption.

hongzhidao commented 1 year ago

Do you mean the bug of undefined behavior happened in place (a)?

    p = msg;
    end = p + sizeof(msg) - 1;

    p = nxt_unit_snprint_prefix(p, end, pid, level);

    va_start(ap, fmt);
    p += vsnprintf(p, end - p, fmt, ap);  /* (a) */
    va_end(ap);

    if (nxt_slow_path(p > end)) {
        memcpy(end - 5, "[...]", 5);
        p = end;
    }

    *p++ = '\n';

In (a), if the return value p is greater than end, it's bad? If yes, it's related to the usage of vsnprintf(). But from the man page, it's allowed to be truncated, what's the issue with it?

alejandro-colomar commented 1 year ago

Do you mean the bug of undefined behavior happened in place (a)?

Yes.

    p = msg;
    end = p + sizeof(msg) - 1;

    p = nxt_unit_snprint_prefix(p, end, pid, level);

    va_start(ap, fmt);
    p += vsnprintf(p, end - p, fmt, ap);  /* (a) */
    va_end(ap);

    if (nxt_slow_path(p > end)) {
        memcpy(end - 5, "[...]", 5);
        p = end;
    }

    *p++ = '\n';
In (a), if the return value p is greater than end, it's bad? If yes, it's related to the usage of vsnprintf(). But from the man page, it's allowed to be truncated, what's the issue with it?

The vsnprintf(3) function truncates the string, but the return value is the length of the total string that it tried to create. For example:

char  buf[3];

p = buf;  // Imagine that p is 0x1000
p += vsnprintf(p, 3, "Hello", ap);  // This returns 5!  However, the string will be copied as "He", which is fine.
// In the previous line, p would be 0x1005, which is an invalid pointer, since it doesn't point to the contents of buf, or one past the end.  That is already UB.

So, the string itself is not a problem, because it's truncated. The problem is in updating the pointer.

hongzhidao commented 1 year ago

I still can't understand what the bug is here.

    p = msg;
    end = p + sizeof(msg) - 1;

    p = nxt_unit_snprint_prefix(p, end, pid, level);

    va_start(ap, fmt);
    p += vsnprintf(p, end - p, fmt, ap);  /* (a) */
    va_end(ap);

    if (nxt_slow_path(p > end)) { /* (b) */
        memcpy(end - 5, "[...]", 5);
        p = end;
    }

    *p++ = '\n';

There is no something like unsafe code with vsnprintf(), correct?
In the above code, if [p, end - p] is truncated, the p is greater than end, but in (b), it has been processed. Is there something I missed?

alejandro-colomar commented 1 year ago

I still can't understand what the bug is here.

I'll reduce the code more:

    p = msg;
    end = p + sizeof(msg) - 1;

    // p = nxt_unit_snprint_prefix(p, end, pid, level);  // Ignore

    va_start(ap, fmt);
    p += vsnprintf(p, end - p, fmt, ap);  /* (a) */
    va_end(ap);

    // if (nxt_slow_path(p > end)) { /* (b) */
    //     memcpy(end - 5, "[...]", 5);
    //     p = end;
    // }

    *p++ = '\n';

1. There is no something like unsafe code with `vsnprintf()`, correct?

2. In the above code, if [p, end - p] is truncated, the `p` is greater than `end`, but in (b), it has been processed.
   Is there something I missed?

Imagine that sizeof(msg) is 3.
Imagine that msg is located at 0x1000.
Then the maximum value allowed for derived pointers is 0x1003.
Then end is 0x1000 + 3 - 1 = 0x1002.
Imagine that fmt is "hello".

With those assumptions, p initially has a value of 0x1000.

The call to vsnprintf() is p += vsnprintf(0x1000, 0x1002 - 0x1000, "hello", ap);

That function writes "he" into p, correctly truncating the string, but it returns 5, the length of the string without truncation.

So, p after the call is 0x1000 + 5 = 0x1005. p is not allowed to have a value outside of [0x1000, 0x1002).

hongzhidao commented 1 year ago

If you are talking about nxt_unit_log(), the msg size is NXT_MAX_ERROR_STR.

alejandro-colomar commented 1 year ago

If you are talking about nxt_unit_log(), the msg size is NXT_MAX_ERROR_STR.

3 was a simplification for showing the problem. If the buffer size is larger, the copied string needs to be larger to reproduce UB, but it's not impossible (or at least, it's not obvious why it would be impossible; other constraints in the program might make it impossible).

hongzhidao commented 1 year ago

the copied string needs to be larger to reproduce UB

Even if the copied string is large enough, it seems it also can't produce a bug here.

alejandro-colomar commented 1 year ago

the copied string needs to be larger to reproduce UB

Even if the copied string is large enough, it seems it also can't produce a bug here.

I'll show a modified version of the example above.

I still can't understand what the bug is here.

I'll reduce the code more:

    p = msg;
    end = p + sizeof(msg) - 1;

    va_start(ap, fmt);
    p += vsnprintf(p, end - p, fmt, ap);  /* (a) */
    va_end(ap);

1. There is no something like unsafe code with `vsnprintf()`, correct?

2. In the above code, if [p, end - p] is truncated, the `p` is greater than `end`, but in (b), it has been processed.
   Is there something I missed?

Imagine that sizeof(msg) is NXT_MAX_ERROR_STR.
Imagine that msg is located at 0x10000.
Then the maximum value allowed for derived pointers is 0x10000 + NXT_MAX_ERROR_STR.
Then end is 0x10000 + NXT_MAX_ERROR_STR - 1.
Imagine that fmt is "hellooooo...(2x-NXT_MAX_ERROR_STR-bytes)oooooo".

With those assumptions, p initially has a value of 0x10000.

The call to vsnprintf() is p += vsnprintf(0x10000, NXT_MAX_ERROR_STR - 1, "hellooooo...(2x-NXT_MAX_ERROR_STR-bytes)oooooo", ap);

That function writes the first NXT_MAX_ERROR_STR bytes into p, correctly truncating the string, but it returns 2 * NXT_MAX_ERROR_STR, the length of the string without truncation.

So, p after the call is 0x10000 + 2 * NXT_MAX_ERROR_STR. p is not allowed to have a value outside of [0x10000, 0x10000 + NXT_MAX_ERROR_STR).

hongzhidao commented 1 year ago

Why rewrite the code?

// if (nxt_slow_path(p > end)) { /* (b) */
    //     memcpy(end - 5, "[...]", 5);
    //     p = end;
    // }

alejandro-colomar commented 1 year ago

Why rewrite the code?

I just copy&pasted. I'll remove it. :)

hongzhidao commented 1 year ago

So, p after the call is 0x10000 + 2 * NXT_MAX_ERROR_STR. p is not allowed to have a value outside of [0x10000, 0x10000 + NXT_MAX_ERROR_STR).

If p is greater than end, it's set to end.

if (nxt_slow_path(p > end)) { /* (b) */
    memcpy(end - 5, "[...]", 5);
    p = end;
}

alejandro-colomar commented 1 year ago

So, p after the call is 0x10000 + 2 * NXT_MAX_ERROR_STR. p is not allowed to have a value outside of [0x10000, 0x10000 + NXT_MAX_ERROR_STR).

If p is greater than end, it's set to end.
if (nxt_slow_path(p > end)) { /* (b) */
    memcpy(end - 5, "[...]", 5);
    p = end;
}

No, that's not true. Where is that code?

vsnprintf(3) from libc doesn't behave like our own nxt_sprintf(). It's very different.

hongzhidao commented 1 year ago

If you are talking about nxt_unit_log(), the msg size is NXT_MAX_ERROR_STR. No, that's not true. Where is that code?

void
nxt_unit_log(nxt_unit_ctx_t *ctx, int level, const char *fmt, ...)
{
    int              log_fd, n;
    char             msg[NXT_MAX_ERROR_STR], *p, *end;
    pid_t            pid;
    va_list          ap;
    nxt_unit_impl_t  *lib;

    if (nxt_fast_path(ctx != NULL)) {
        lib = nxt_container_of(ctx->unit, nxt_unit_impl_t, unit);

        pid = lib->pid;
        log_fd = lib->log_fd;

    } else {
        pid = nxt_unit_pid;
        log_fd = STDERR_FILENO;
    }

    p = msg;
    end = p + sizeof(msg) - 1;

    p = nxt_unit_snprint_prefix(p, end, pid, level);

    va_start(ap, fmt);
    p += vsnprintf(p, end - p, fmt, ap);
    va_end(ap);

    if (nxt_slow_path(p > end)) {
        memcpy(end - 5, "[...]", 5);
        p = end;
    }

    *p++ = '\n';

    n = write(log_fd, msg, p - msg);
    if (nxt_slow_path(n < 0)) {
        fprintf(stderr, "Failed to write log: %.*s", (int) (p - msg), msg);
    }
}

alejandro-colomar commented 1 year ago

If you are talking about nxt_unit_log(), the msg size is NXT_MAX_ERROR_STR. No, that's not true. Where is that code?

void
nxt_unit_log(nxt_unit_ctx_t *ctx, int level, const char *fmt, ...)
{
    int              log_fd, n;
    char             msg[NXT_MAX_ERROR_STR], *p, *end;
    pid_t            pid;
    va_list          ap;
    nxt_unit_impl_t  *lib;

    if (nxt_fast_path(ctx != NULL)) {
        lib = nxt_container_of(ctx->unit, nxt_unit_impl_t, unit);

        pid = lib->pid;
        log_fd = lib->log_fd;

    } else {
        pid = nxt_unit_pid;
        log_fd = STDERR_FILENO;
    }

    p = msg;
    end = p + sizeof(msg) - 1;

    p = nxt_unit_snprint_prefix(p, end, pid, level);

    va_start(ap, fmt);
    p += vsnprintf(p, end - p, fmt, ap);
    va_end(ap);

    if (nxt_slow_path(p > end)) {
        memcpy(end - 5, "[...]", 5);
        p = end;
    }

    *p++ = '\n';

    n = write(log_fd, msg, p - msg);
    if (nxt_slow_path(n < 0)) {
        fprintf(stderr, "Failed to write log: %.*s", (int) (p - msg), msg);
    }
}

Ahhh, that's after Undefined Behavior has already happened. Anythign after UB has been invoked doesn't return to a valid state.

hongzhidao commented 1 year ago

Please show what the function has bug, I'm a bit confused.

alejandro-colomar commented 1 year ago

If you are talking about nxt_unit_log(), the msg size is NXT_MAX_ERROR_STR. No, that's not true. Where is that code?

void
nxt_unit_log(nxt_unit_ctx_t *ctx, int level, const char *fmt, ...)
{
    int              log_fd, n;
    char             msg[NXT_MAX_ERROR_STR], *p, *end;
    pid_t            pid;
    va_list          ap;
    nxt_unit_impl_t  *lib;

    if (nxt_fast_path(ctx != NULL)) {
        lib = nxt_container_of(ctx->unit, nxt_unit_impl_t, unit);

        pid = lib->pid;
        log_fd = lib->log_fd;

    } else {
        pid = nxt_unit_pid;
        log_fd = STDERR_FILENO;
    }

    p = msg;
    end = p + sizeof(msg) - 1;

    p = nxt_unit_snprint_prefix(p, end, pid, level);

    va_start(ap, fmt);
    p += vsnprintf(p, end - p, fmt, ap);

Undefined Behavior has been invoked in p += ....

Anything after this point is already invalid, and can't make the program return to a valid behavior.

    va_end(ap);

    if (nxt_slow_path(p > end)) {
        memcpy(end - 5, "[...]", 5);
        p = end;
    }

    *p++ = '\n';

    n = write(log_fd, msg, p - msg);
    if (nxt_slow_path(n < 0)) {
        fprintf(stderr, "Failed to write log: %.*s", (int) (p - msg), msg);
    }
}
Ahhh, that's after Undefined Behavior has already happened. Anythign after UB has been invoked doesn't return to a valid state.

nginx / unit

Undefined Behavior: write past the end of an array #795