Fix snprintf() overflow issues
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 3 May 2017 16:25:08 +0000 (12:25 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Thu, 4 May 2017 18:44:03 +0000 (14:44 -0400)
snprintf() is a pain.  Taking a 'negative' number for consecutive calls
after an overflow is a common pattern: see netif.c (derived from Plan 9)
and mlx4/mcg.c (from Linux).  We had been taking an int for the size, but
ought to be taking a size_t - if for nothing else than to make the change
now *and* adding the check, instead of later without the check.

On a similar note seprintf() was using its return value incorrectly.  The
uses of seprintf() seemed ok, since no one used "buf + rc" for anything
other than 'start' for another seprintf() call.  We could have seprintf()
do the same thing as snprintf(), but it seems a little dangerous to return
a pointer outside the [start, end) range.

One thing to remember is that if seprintf()/snprintf() overflowed, we
probably don't have a null-terminated buffer.  Some of our call sites might
not like that.

Similarly, the logic in parlib for detecting overflow was just wrong.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/include/stdio.h
kern/src/printfmt.c
tests/get_html.c
tests/srv.c
user/parlib/include/parlib/net.h
user/parlib/net.c
user/utest/devvars.c

index aa880e6..e06e920 100644 (file)
@@ -34,8 +34,14 @@ int  ( cprintf)(const char *fmt, ...);
 int    vcprintf(const char *fmt, va_list);
 
 // lib/sprintf.c
-int    snprintf(char *str, int size, const char *fmt, ...);
-int    vsnprintf(char *str, int size, const char *fmt, va_list);
+
+static inline bool snprintf_error(int ret, size_t buf_len)
+{
+       return ret < 0 || ret >= buf_len;
+}
+
+int snprintf(char *str, size_t size, const char *fmt, ...);
+int vsnprintf(char *str, size_t size, const char *fmt, va_list);
 
 // lib/fprintf.c
 int    printf(const char *fmt, ...);
index eec17e6..b64625e 100644 (file)
@@ -273,13 +273,22 @@ static void sprintputch(int ch, sprintbuf_t **b)
        (*b)->cnt++;
 }
 
-int vsnprintf(char *buf, int n, const char *fmt, va_list ap)
+int vsnprintf(char *buf, size_t n, const char *fmt, va_list ap)
 {
        sprintbuf_t b;// = {buf, buf+n-1, 0};
        sprintbuf_t *bp = &b;
 
        /* this isn't quite the snprintf 'spec', but errors aren't helpful */
-       if (buf == NULL || n < 1)
+       assert(buf);
+       /* We might get large, 'negative' values for code that repeatedly calls
+        * snprintf(), e.g.:
+        *              len += snprintf(buf + len, bufsz - len, "foo");
+        *              len += snprintf(buf + len, bufsz - len, "bar");
+        * If len > bufsz, that will appear as a large value.  This is not quite the
+        * glibc semantics (we aren't returning the size we would have printed), but
+        * it short circuits the rest of the function and avoids potential errors in
+        * the putch() functions. */
+       if (!n || (n > INT32_MAX))
                return 0;
 
        b.buf = NULL; // zra : help out the Deputy optimizer a bit
@@ -295,7 +304,7 @@ int vsnprintf(char *buf, int n, const char *fmt, va_list ap)
        return b.cnt;
 }
 
-int snprintf(char *buf, int n, const char *fmt, ...)
+int snprintf(char *buf, size_t n, const char *fmt, ...)
 {
        va_list ap;
        int rc;
@@ -307,22 +316,29 @@ int snprintf(char *buf, int n, const char *fmt, ...)
        return rc;
 }
 
-/* convenience function: do a print, return the pointer to the end. */
+/* Convenience function: do a print, return the pointer to the null at the end.
+ *
+ * Unlike snprintf(), when we overflow, this doesn't return the 'end' where we
+ * would have written to.  Instead, we'll return 'end - 1', which is the last
+ * byte, and enforce the null-termination.  */
 char *seprintf(char *buf, char *end, const char *fmt, ...)
 {
        va_list ap;
        int rc;
-       int n = end - buf;
-
-       if (n <= 0)
-               return buf;
+       size_t n = end - buf;
 
        va_start(ap, fmt);
        rc = vsnprintf(buf, n, fmt, ap);
        va_end(ap);
 
-       if (rc >= 0)
-               return buf + rc;
-       else
+       /* Some error - leave them where they were. */
+       if (rc < 0)
                return buf;
+       /* Overflow - put them at the end */
+       if (rc >= n) {
+               *(end - 1) = '\0';
+               return end - 1;
+       }
+       assert(buf[rc] == '\0');
+       return buf + rc;
 }
index 45b4b88..43a3a58 100644 (file)
@@ -38,7 +38,7 @@ int main(int argc, char *argv[])
        printf("Trying to access http://%s:%s/%s\n", host, port, page);
        /* manually making our own addr (no mkaddr, which was racy anyway) */
        ret = snprintf(addr, sizeof(addr), "tcp!%s!%s", host, port);
-       if (snprintf_overflow(ret, addr, sizeof(addr))) {
+       if (snprintf_error(ret, sizeof(addr))) {
                perror("Addr string too long");
                exit(-1);
        }
index 073f25d..cfa352e 100644 (file)
@@ -39,7 +39,7 @@ int main(int argc, char *argv[])
                exit(-1);
        }
        ret = snprintf(buf, buf_len, "#srv/%s", srvname);
-       if (snprintf_overflow(ret, buf, buf_len)) {
+       if (snprintf_error(ret, buf_len)) {
                printf("srvname too long\n");
                exit(-1);
        }
index d72b31e..8e055d0 100644 (file)
@@ -6,11 +6,13 @@
 
 #pragma once
 
+#include <stdbool.h>
+
 __BEGIN_DECLS
 
-static inline int snprintf_overflow(int ret, char *buf, size_t buf_len)
+static inline bool snprintf_error(int ret, size_t buf_len)
 {
-       return (ret == buf_len) && (buf[buf_len - 1] != 0);
+       return ret < 0 || ret >= buf_len;
 }
 
 int cheap_dial(char *addr, char *local, char *dir, int *cfdp);
index de449f3..5e780d3 100644 (file)
@@ -42,7 +42,7 @@ int cheap_dial(char *addr, char *local, char *dir, int *cfdp)
        *hostname++ = '\0';
        prefix = (addr[0] == '/' ? "" : "/net/");
        ret = snprintf(buf, buf_len, "%s%s/clone", prefix, addr);
-       if (snprintf_overflow(ret, buf, buf_len)) {
+       if (snprintf_error(ret, buf_len)) {
                perror("Clone chan path too long");
                ret = -1;
                goto out_readdr;
@@ -65,7 +65,7 @@ int cheap_dial(char *addr, char *local, char *dir, int *cfdp)
        buf[ret] = 0;
        conv_id = atoi(buf);
        ret = snprintf(buf, buf_len, "connect %s", hostname);
-       if (snprintf_overflow(ret, buf, buf_len)) {
+       if (snprintf_error(ret, buf_len)) {
                perror("Connect string too long");
                ret = -1;
                goto out_ctlfd;
@@ -76,7 +76,7 @@ int cheap_dial(char *addr, char *local, char *dir, int *cfdp)
                goto out_ctlfd;
        }
        ret = snprintf(buf, buf_len, "%s%s/%d/data", prefix, addr, conv_id);
-       if (snprintf_overflow(ret, buf, buf_len)) {
+       if (snprintf_error(ret, buf_len)) {
                perror("Data chan path too long");
                ret = -1;
                goto out_ctlfd;
index 1806454..f5e9f1e 100644 (file)
@@ -18,7 +18,7 @@ static bool test_read_var(const char *name, const char *val)
        int fd, ret;
 
        ret = snprintf(buf, sizeof(buf), "#vars/%s", name);
-       if (snprintf_overflow(ret, buf, sizeof(buf)))
+       if (snprintf_error(ret, sizeof(buf)))
                UT_ASSERT_FMT("snprintf failed!", FALSE);
        fd = open(buf, O_READ);
        UT_ASSERT_FMT("Could not open vars file %s, check CONFIG_DEVVARS_TEST",
@@ -78,7 +78,7 @@ static bool test_new_var(const char *name, const char *val)
        int fd, ret;
 
        ret = snprintf(path, sizeof(path), "#vars/%s", name);
-       if (snprintf_overflow(ret, path, sizeof(path)))
+       if (snprintf_error(ret, sizeof(path)))
                UT_ASSERT_FMT("snprintf failed!", FALSE);
        fd = open(path, O_READ | O_CREATE, S_IRUSR);
        UT_ASSERT_FMT("Could not open vars file %s, check CONFIG_DEVVARS_TEST",