valgrind error with strcpy

Question

This code is supposed to extract a path from the userInput null-terminated string

  /* begin createPath */
    static inline char* createPath(char * userInput)
    {/* This function retuns the path from the userInput */
            int pathStringLength = 0;
            char *buf = userInput;
            while(*(buf++) != ' ')
                    pathStringLength++;
            char *path = malloc(pathStringLength+1);
            strncpy(path, userInput, pathStringLength);
    //      memcpy(path, userInput, pathStringLength);
            path[pathStringLength+1] = '\0';        
            return path;
    }
    /* end createPath */

According to valgrind, this code has an error:

> ==2919== Conditional jump or move depends on uninitialised value(s)
> ==2919==    at 0x400A87: createPath (in /home/aral/learn/myShell/myShell)
> ==2919==    by 0x400A4C: parseInput (in /home/aral/learn/myShell/myShell)
> ==2919==    by 0x4009C3: main (in /home/aral/learn/myShell/myShell)
> ==2919== 
> ==2919== Invalid write of size 1
> ==2919==    at 0x400AC3: createPath (in /home/aral/learn/myShell/myShell)
> ==2919==    by 0x400A4C: parseInput (in /home/aral/learn/myShell/myShell)
> ==2919==    by 0x4009C3: main (in /home/aral/learn/myShell/myShell)

Searching on stackoverflow for similar problems, some people talked about adding a null-terminator while others mentioned using memcpy instead of strcpy; I am adding a null anyway and I tried to use memcpy but nothing improved, valgrind kept complaining.

What am I doing wrong here exactly? and how can I fix it?

Answer 1

path[pathStringLength+1] = '\0';

is wrong. That's one byte off the end. You mean:

path[pathStringLength] = '\0';

You will also have a buffer overrun if the input string does not have a space. Check for the null terminator in your loop and terminate when you encounter one. I'd write it like this:

while (*buf != ' ' && *buf != '\0')
{
    pathStringLength++;
    buff++;
}

For what it is worth, I think memcpy is probably a better option here. Once you've worked out exactly how much text needs to be copied, you may as well just blit it over. No need for a string function which looks for null terminators. You, once you fix the code, have already checked for them.

And you should check the return value of malloc.