Split a char array with NUL terminators and inner delimiters

Question

I get as an input a char array in this format: "key = value\0key = value...\0" (last pair is ended with 2 NUL terminators - one is the pair end + one is the whole string end)

and I want to split it into pairs of {key, value} in order to print them.

This is my approach; is there a better way (strtok-like) to do so?

char str [] =   "line 1 = But I set fire to the rain\0line 2 = Watched it pour as I touched your face\0line 3 = Well, it burned while I cried\0line 4 = 'Cause I heard it screaming out your name, your name!\0";
char * p = str;
char key[10], value[128], * delimiter;

while(*p){
    memset(key,     0, sizeof(key));
    memset(value,   0, sizeof(value));

    delimiter = strchr(p, '=');
    strncpy(key, p, delimiter - p);
    // not sure where the NUL terminator is, so use strcpy
    strcpy(value, 1 + delimiter);

    printf("key: %s\n   value: %s\n", key, value);
    p += strlen(p) + 1;
}

I'm copying the pairs into temp arrays so I could process them later...

Answer 1

// not sure where the NUL terminator is, so use strcpy
strcpy(value, 1 + delimiter);

This is really dangerous. strncpy exists to prevent the possibility of buffer overflows, which are a risk precisely when you don't know how much memory you're throwing around.

If you are copying to an array of fixed size and you don't know how much to allow it to copy, use strncpy and restrict it to copying up to the length of the detination buffer. Note: strncpy(dest, src, length) copies at most length characters, or up to a null terminator if sooner. So that will not guarantee that your program works correctly (and in particular it will not guarantee that your destination is a valid null-terminated C string). It will prevent a buffer overflow if something goes wrong, which would be a much harder problem to handle.

It would still be better if you did know ahead of time how much to expect to copy, because that allows you to handle failure cases. You can do this: just use strlen(delimiter). In fact, you're already doing this! You're just doing it to p and two lines too late.

However, strncpy is not a magic bullet if used incorrectly. strncpy(key, p, delimiter - p); is vulnerable to overflow because it does not consider the capacity of key. If delimiter - p is say 15 then that line says "Copy from here into this array of size 10, but not more than 15 characters."

Take home is always know the sizes of arrays that you're dealing with, and ritually prefer the functions that check sizes such as strncpy(dst, src, sizeof(dst)) as a second line of defence so that if you do have a bug in handling array lengths the bug is less likely to be of the worst case security vulnerability sort.

---

A second concern I have with this code is that it relies on str being double null terminated. This is an unusual convention, and so requires a lot of conscious effort to get right. If you pack this into a function and it gets called by anyone else, or even by yourself when you're thinking about the logic of the calling component rather than the splitting component, you are going to keep reading through a bunch of junk.

Even worse, because it is not unusual for debug builds to be more generous with initialising stuff to zero than release builds, you stand a very real risk that it will appear to work fine for ages until you push it out to someone else!

---

The third thing to mention, and a general mindset one, is be careful not to assume that you are on the so called "happy path". Your code should do the right thing if given inputs in a form you expect, and it does. It should also confirm whereever possible that the inputs are in the form you expect.

For example, consider delimiter = strchr(p, '='); Great, finds the "=" character to split on. If this bit of the string does not contain "=" anywhere, it returns NULL. You should check that this has not happened. Of course delimiter - p where delimiter is NULL is going to have some weird behaviour! You should check that any functions which might fail have not failed before relying on their output.

Answer 2

In addition to Josiah's review:

The input str is a non-const array. If you don't need to keep the original contents, then it's easier and faster to overwrite in place:

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

int main()
{
    char str[] =
        "line 1 = But I set fire to the rain\0"
        "line 2 = Watched it pour as I touched your face\0"
        "line 3 = Well, it burned while I cried\0"
        "line 4 = 'Cause I heard it screaming out your name, your name!\0";


    for (char *p = str;  *p;  p += strlen(p) + 1) {
        char *delimiter = strchr(p, '=');
        if (delimiter) {
            *delimiter = '\0';
            const char *const key = p;
            const char *const value = p = delimiter + 1;

            printf("key: %s\n   value: %s\n", key, value);
        } else {
            fprintf(stderr, "No '=' in string \"%s\"\n", p);
        }
    }
}

If you do need to keep the original value, then the same technique can be used after copying the string to newly-allocated memory.

You might want to consider whether it's correct to keep the whitespace either side of the delimiter - from the example, it appears that it should be removed (by backing up delimiter with while (isspace(*d)) --d; and advancing 'value' the same way).