-
Notifications
You must be signed in to change notification settings - Fork 323
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
Undefined Behavior: write past the end of an array #795
Comments
However, that doesn't fix the other important problem with this code: we're not really detecting string truncation at all.
|
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 |
Also, copying with |
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, Basically, |
The following function is better than /* 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 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. |
Hi,
|
Hmm, yes, this case can be fixed with In the case of @ac000 's code, that solution is not valid, because
|
So,
|
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 $ 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 Again, the underlying issue is concatenating We need to add a wrapper that has similar semantics as stpecpy() (proposed in #805), which I'd call |
Suggestion for a better replacement for $ 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 |
Do you mean the bug of undefined behavior happened in place (a)?
In (a), if the return value |
Yes.
The 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. |
I still can't understand what the bug is here.
|
I'll reduce the code more:
With those assumptions, The call to That function writes So, |
If you are talking about |
|
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'll reduce the code more:
With those assumptions, The call to That function writes the first So, |
Why rewrite the code?
|
I just copy&pasted. I'll remove it. :) |
If
|
No, that's not true. Where is that code?
|
|
Ahhh, that's after Undefined Behavior has already happened. Anythign after UB has been invoked doesn't return to a valid state. |
Please show what the function has bug, I'm a bit confused. |
Undefined Behavior has been invoked in Anything after this point is already invalid, and can't make the program return to a valid behavior.
|
While trying to fix a bug, I found these:
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.
The text was updated successfully, but these errors were encountered: