path_utils: path_concat should return empty string on ENOBUFSding_libs-0-1

Also clean up the code a bit and add more comments

3 files changed, 55 insertions, 15 deletions

diff --git a/path_utils/path_utils.c b/path_utils/path_utils.c
index 25ca426..0a758c5 100644
--- a/path_utils/path_utils.c
+++ b/path_utils/path_utils.c
@@ -192,6 +192,7 @@ bool is_absolute_path(const char *path)
 
 int path_concat(char *path, size_t path_size, const char *head, const char *tail)
 {
+    int ret;
     const char *p, *src;
     char *dst, *dst_end;
 
@@ -201,13 +202,27 @@ int path_concat(char *path, size_t path_size, const char *head, const char *tail
     dst_end = path + path_size - 1;             /* -1 allows for NULL terminator */
 
     if (head && *head) {
-        for (p = head; *p; p++);                /* walk to end of head */
-        for (p--; p > head && *p == '/'; p--); /* skip any trailing slashes in head */
-        if ((p - head) > path_size-1) return ENOBUFS;
-        for (src = head; src <= p && dst < dst_end;) *dst++ = *src++; /* copy head */
+        /* walk to end of head */
+        for (p = head; *p; p++);
+
+        /* skip any trailing slashes in head */
+        for (p--; p > head && *p == '/'; p--);
+
+        /* If the length of head exceeds the buffer size, fail */
+        if ((p - head) > path_size-1) {
+            ret = ENOBUFS;
+            goto fail;
+        }
+
+        /* Copy head into path */
+        for (src = head; src <= p && dst < dst_end;) {
+            *dst++ = *src++;
+        }
     }
     if (tail && *tail) {
-        for (p = tail; *p && *p == '/'; p++);   /* skip any leading slashes in tail */
+        /* skip any leading slashes in tail */
+        for (p = tail; *p && *p == '/'; p++);
+
         if (dst > path)
             /* insert single slash between head & tail
              * Making sure not to add an extra if the
@@ -218,15 +233,28 @@ int path_concat(char *path, size_t path_size, const char *head, const char *tail
             if (dst < dst_end && *(dst-1) != '/') {
                 *dst++ = '/';
             }
-        for (src = p; *src && dst < dst_end;) *dst++ = *src++; /* copy tail */
-        if (*src) return ENOBUFS; /* failed to copy everything */
+
+        /* Copy the tail into the path */
+        for (src = p; *src && dst < dst_end;) {
+            *dst++ = *src++;
+        }
+
+        /* If we got past dst_end and there is more data
+         * in the src buffer, we should return ENOBUFS
+         */
+        if (*src) {
+            ret = ENOBUFS; /* failed to copy everything */
+            goto fail;
+        }
     }
     *dst = 0;
-    if (dst > dst_end) {
-        return ENOBUFS;
-    }
+
     return SUCCESS;
 
+fail:
+    /* On failure, set the buffer to the empty string for safety */
+    *path = '\0';
+    return ret;
 }
 
 int make_path_absolute(char *absolute_path, size_t absolute_path_size, const char *path)
diff --git a/path_utils/path_utils.h b/path_utils/path_utils.h
index 2fd567c..17ed1f4 100644
--- a/path_utils/path_utils.h
+++ b/path_utils/path_utils.h
@@ -198,7 +198,8 @@ bool is_absolute_path(const char *path);
  * @param[in]    tail           The second component of the path
  *
  * @return \c SUCCESS if successful, non-zero error code otherwise.
- * \li \c ENOBUFS      If the buffer space is too small
+ * \li \c ENOBUFS      If the buffer space is too small. In this case,
+ *                     path will be set to an empty string.
  */
 int path_concat(char *path, size_t path_size, const char *head, const char *tail);
 
diff --git a/path_utils/path_utils_ut.c b/path_utils/path_utils_ut.c
index 34462cf..aadd89b 100644
--- a/path_utils/path_utils_ut.c
+++ b/path_utils/path_utils_ut.c
@@ -252,24 +252,35 @@ START_TEST(test_path_concat_neg)
     char small[3];
     char small2[5];
     char small3[7];
-    char p2[9];
+    char p2[10];
 
     /* these two test different conditions */
 
     /* Test if head is longer than the buffer */
     fail_unless(path_concat(small, 3, "/foo", "bar") == ENOBUFS);
+    /* On ENOBUFS, path should be empty */
+    fail_unless_str_equal(small, "");
 
     /* Test if head is the same length as the buffer */
     fail_unless(path_concat(small2, 5, "/foo", "bar") == ENOBUFS);
+    /* On ENOBUFS, path should be empty */
+    fail_unless_str_equal(small2, "");
 
     /* Test if head+tail is the longer than the buffer */
     fail_unless(path_concat(small3, 7, "/foo", "bar") == ENOBUFS);
+    /* On ENOBUFS, path should be empty */
+    fail_unless_str_equal(small3, "");
 
     /* off-by-one */
-    p2[8] = 1; /* Check whether we've written too far */
+    /* Fill with garbage data for now */
+    memset(p2, 'Z', 9);
+    p2[9] = '\0';
+
     fail_unless(path_concat(p2, 8, "/foo", "bar") == ENOBUFS);
-    fail_unless(p2[8] == 1); /* This should be untouched */
-    fail_unless_str_equal(p2, "/foo/ba");
+    /* Make sure we don't write past the end of the buffer */
+    fail_unless(p2[8] == 'Z', "Got [%d]", p2[8]);
+    /* On ENOBUFS, path should be empty */
+    fail_unless_str_equal(p2, "");
 }
 END_TEST