Correct string length handling in strlimcpy in halcmd.c

[petterreinholdtsen]

The strlen value is the string length, often fetched from strlen(),
which return the number of characters in the string, excluding the
terminal NUL character. To have room to copy the string into *dest,
the *destspace available must be strlen+1, not strlen, to also have
room for the NUL at the end. Setting NUL explicitly will not be
needed in the case srclen reflect the real length of str, but if srclen
is smaller than the real length of str, will will ensure the string
is always NUL terminated. Pass *destspace instead of strlen+1 to
strncpy() ensure the actual target size limit is used no matter what
the content of srclen is.

[jepler]

You're right. I made a standalone test program for this just so I could be sure.

#include <stdio.h>
#include <string.h>

static int strlimcpy(char **dest, const char *src, int srclen, int *destspace) {
    if (*destspace < srclen) {
        return -1;
    } else {
        strncpy(*dest, src, srclen);
        (*dest)[srclen] = '\0';
        srclen = strlen(*dest);         /* use the actual number of bytes copied */
        *destspace -= srclen;
        *dest += srclen;
    }
    return 0;
}
                        

static int strlimcpy2(char **dest, const char *src, int srclen, int *destspace) {
    if (*destspace < srclen + 1) {
        return -1;
    } else {
        strncpy(*dest, src, *destspace);
        (*dest)[srclen] = '\0';
        srclen = strlen(*dest);         /* use the actual number of bytes copied */
        *destspace -= srclen;
        *dest += srclen;
    }
    return 0;
}
                        
int main() {
    const char src[] = "too big?";
    char buf[8];

    char *bufptr = buf;
    int space = sizeof(buf);

    int result = strlimcpy2(&bufptr, src, strlen(src), &space);
    if(result < 0) { printf("failed\n"); }
    else {
        for(int i=0; i<sizeof(buf); i++) {
            printf("buf[%d] = %d\n", i, (unsigned char)buf[i]);
        }
    }
    printf("\n\n");

    bufptr = buf;
    space = sizeof(buf);

    result = strlimcpy(&bufptr, src, strlen(src), &space);
    if(result < 0) { printf("failed\n"); }
    else {
        for(int i=0; i<sizeof(buf); i++) {
            printf("buf[%d] = %d\n", i, (unsigned char)buf[i]);
        }
    }
}

With gcc -fsanitize=address this got a nice diagnostic on the call of the unmodified strlimcpy:

==2295289==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe7febd3f8 at pc 0x55ecfdfaa37c bp 0x7ffe7febd380 sp 0x7ffe7febd378
WRITE of size 1 at 0x7ffe7febd3f8 thread T0
    #0 0x55ecfdfaa37b in strlimcpy /home/jepler/slc.c:9
    #1 0x55ecfdfaa584 in main /home/jepler/slc.c:50
    #2 0x7f071af1ed09 in __libc_start_main ../csu/libc-start.c:308
    #3 0x55ecfdfaa169 in _start (/home/jepler/a.out+0x1169)

Address 0x7ffe7febd3f8 is located in stack of thread T0 at offset 72 in frame
    #0 0x55ecfdfaa392 in main /home/jepler/slc.c:31

  This frame has 4 object(s):
    [48, 52) 'space' (line 36)
    [64, 72) 'buf' (line 33) <== Memory access at offset 72 overflows this variable
    [96, 104) 'bufptr' (line 35)
    [128, 137) 'src' (line 32)

The improved version of the function correctly returns -1 to indicate failure.