Fix buffer overflow in cat_with

Account for separators when computing the buffer size in cat_width in order
to avoid overflowing the buffer.

Previously, cat_width only accounted for the lengths of the strings being
catenated, but not the separators, which could result in buffer overflows.
For example, trace the execution of the following invocation of cat_with:

    char s[5] = "xxxx";
    cat_with(' ', s, s, s, s, s, s, s, s);

cat_with initialises size and length and allocates an initial buffer:

    size = 32;                       /* size = 32                  */
    length = 0;                      /* length = 0                 */
    buf = calloc(1, size + 1);       /* allocated 33 bytes for buf */

The first iteration of the main loop in cat_with catenates the first string
and a separator but fails to account for the separator in length:

    length += strlen(curr);          /* length = 4                 */
    while (length + 2 > size)        /* 4 + 2 > 32 is false        */
    strcat(buf, curr);               /* written 5 bytes to buf     */
    strcat(buf, split);              /* written 6 bytes to buf     */

Now we have written the string "xxxx " (6 bytes including NUL) to buf, but
length only counts the 4 bytes "xxxx" (excluding NUL).

In successive iterations, the discrepancy between the value in length and
the actual length of the string in the buffer grows:

    length += strlen(curr);          /* length = 8                 */
    while (length + 2 > size)        /* 8 + 2 > 32 is false        */
    strcat(buf, curr);               /* written 10 bytes to buf    */
    strcat(buf, split);              /* written 11 bytes to buf    */

Now we have written "xxxx xxxx " (11 bytes incl. NUL).

    length += strlen(curr);          /* length = 12                */
    while (length + 2 > size)        /* 12 + 2 > 32 is false       */
    strcat(buf, curr);               /* written 15 bytes to buf    */
    strcat(buf, split);              /* written 16 bytes to buf    */

Now we have written "xxxx xxxx xxxx " (16 bytes incl. NUL).

    length += strlen(curr);          /* length = 16                */
    while (length + 2 > size)        /* 16 + 2 > 32 is false       */
    strcat(buf, curr);               /* written 20 bytes to buf    */
    strcat(buf, split);              /* written 21 bytes to buf    */

Now we have written "xxxx xxxx xxxx xxxx " (21 bytes incl. NUL).

    length += strlen(curr);          /* length = 20                */
    while (length + 2 > size)        /* 20 + 2 > 32 is false       */
    strcat(buf, curr);               /* written 25 bytes to buf    */
    strcat(buf, split);              /* written 26 bytes to buf    */

Now we have written "xxxx xxxx xxxx xxxx xxxx " (26 bytes incl. NUL).

    length += strlen(curr);          /* length = 24                */
    while (length + 2 > size)        /* 24 + 2 > 32 is false       */
    strcat(buf, curr);               /* written 30 bytes to buf    */
    strcat(buf, split);              /* written 31 bytes to buf    */

Now we have written "xxxx xxxx xxxx xxxx xxxx xxxx " (31 bytes incl. NUL).
Eventually, the string can grow beyond the buffer before length grows large
enough to cause a re-allocation:

    length += strlen(curr);          /* length = 28                */
    while (length + 2 > size)        /* 28 + 2 > 32 is false       */
    strcat(buf, curr);               /* written 35 bytes to buf    */
    strcat(buf, split);              /* written 36 bytes to buf    */

Note that we have not increased size or re-allocated the buffer, and so we
have written "xxxx xxxx xxxx xxxx xxxx xxxx xxxx" (35 bytes incl. NUL) to
a buffer for which we have allocated only 33 bytes.

1 files changed, 3 insertions, 1 deletions

diff --git a/brightnessctl.c b/brightnessctl.c
index 355fc86..60ca3a4 100644
--- a/brightnessctl.c
+++ b/brightnessctl.c
@@ -463,8 +463,10 @@ char *_cat_with(char c, ...) {
 		while (length + 2 > size)
 			buf = realloc(buf, size *= 2);
 		strcat(buf, curr);
-		if ((curr = va_arg(va, char*)))
+		if ((curr = va_arg(va, char*))) {
+			length++;
 			strcat(buf, split);
+		}
 	}
 	return buf;
 }