C Strcat valgrind error

I'm trying to concatenate two strings so I can obtain a file path. However, I'm receiving an error in valgrind

Conditional jump or move depends on uninitialised value(s)

My code:

/**
 * @brief Concatenate two strings to get file path
 * @param firstP - First string
 * @param secondP - Second string
 * @return Returns the concatenated string
 */
char *getPathDir(char *firstP, char *secondP) {
    char *new_str;
    int stringSize = strlen(firstP)+strlen(secondP)+2;

    if((new_str = malloc(stringSize)) != NULL){
        new_str[0] = '\0';
        strcat(new_str,firstP);
        new_str[strlen(firstP)] = '/';
        strcat(new_str,secondP);
    } else {
        perror("malloc");
        cleanUp();
        exit(EXIT_FAILURE);
    }
    return new_str;
}

Solution

Let's look at these lines:

    new_str[0] = '\0';
    strcat(new_str,firstP);
    new_str[strlen(firstP)] = '/';
    strcat(new_str,secondP);

Prior to writing anything, the string looks like this:

     +---+---+---+---+---+---+---+---+
     | ? | ? | ? | ? | ? | ? | ? | ? |
     +---+---+---+---+---+---+---+---+

After the first line (new_str[0] = '\0';), you have this:

     +---+---+---+---+---+---+---+---+
     | 0 | ? | ? | ? | ? | ? | ? | ? |
     +---+---+---+---+---+---+---+---+

After the second line (strcat(new_str,firstP);), it looks like this:

     +---+---+---+---+---+---+---+---+
     | A | B | C | D | 0 | ? | ? | ? |
     +---+---+---+---+---+---+---+---+

Now, when you execute the line

    new_str[strlen(firstP)] = '/';

you overwrite the null terminator and get this:

     +---+---+---+---+---+---+---+---+
     | A | B | C | D | / | ? | ? | ? |
     +---+---+---+---+---+---+---+---+

This is a problem, because your string is no longer null-terminated, so when you next call strcat the program will start reading into uninitialized memory hunting around for a null terminator.

If you want to concatenate the strings together, it might be easier to just use sprintf, like this:

sprintf(new_str, "%s/%s", firstP, secondP);

This more explicitly says "write the first string, then the separator, then the second string" and offloads all the null terminator management to the library. And libraries, with the exception of strncat, typically handle null terminators pretty well. :-)

There's also a chance that the sprintf might be marginally faster than what you're doing. Using lots of strcats in a row in the way you're proposing can be inefficient due to the overhead of rescanning the strings to find the null terminators, but I wouldn't bet on it. It does, however, have the very clear advantage of more accurately communicating what it is that you're trying to do, and readability wins are rarely a bad thing.