"Free heap block modified after it was freed" when modifying string after malloc

Question

I am currently working on a project that includes some file I/O.

Due to it being cross platform I needed to account for different path separators and hence decided to create the following function to simplify the process of joining paths:

/**
 * @brief Joins two paths together.
 * 
 * path = a + seperator + b + '\0'
 * 
 * @param a First path.
 * @param b Second path.
 * 
 * @return A pointer to the two paths combined.
 */
char * join_paths(const char * a, const char * b)
{
    const char separator[] =
    #ifdef _WIN32
        "\\";
    #else
        "/";
    #endif


    /* Get the sizes of all the strings
       to join.                         */
    size_t a_len = strlen(a);
    size_t b_len = strlen(b);
    size_t s_len = strlen(separator);
    size_t total_len = a_len + b_len + s_len + 1; /* +1 for \0 */

    /* path will contain a, b, the separator and \0
       hence the total length is total_len.
    */
    char * path = malloc(total_len);
    check( path != NULL,
           "Error allocating memory for path. " );

    memcpy(path, a, a_len);          /* path will begin with a */

    strncat(path, separator, s_len); /* copy separator */
    strncat(path, b, b_len);         /* copy b         */

error:

    return path;
}

(Where check is a macro from here: http://pastebin.com/2mZxSX3S)

Which had been working fine until now, when I had to use GDB to debug something not related to file I/O and realized that all my file paths seemed to be corrupt (e.g. "¡║¯½½½½½½½½■¯■¯■¯■").

Also I got the error message:

"Free heap block modified after it was freed"

After further investigation I realized this all happens after the memcpy in join_paths.

However this all seems to only happen when running it from GDB. What is going wrong here?

Answer 1

Your memcpy did not zero-terminate the target buffer. Meanwhile, strncat requires a valid string as the target. Without proper zero-termination your strncat calls began concatenation at some unpredictable spot further down the buffer, eventually running over the end of the buffer.

Quite possibly you can fix it by just doing

memcpy(path, a, a_len + 1);

to ensure the terminating zero is also copied.

But why are you mixing mem... functions and str... functions? It is possible to do it properly, but you have to be constantly paying attention to such things as proper zero-termination before you use any str... function.

One can argue that when the length of the string is already known, mem... functions are more appropriate and efficient than str.. functions. In that case, just stick to mem... functions. For example, in your case this should work properly

memcpy(path, a, a_len);
memcpy(path + a_len, separator, s_len);
memcpy(path + a_len + s_len, b, b_len + 1);

Alternatively (and maybe even better), you can use sprintf (or snprintf) to do it in one line

size_t n_written = sprintf(path, "%s%s%s", a, separator, b);
assert(n_written + 1 == total_len);