Unitialized Heap Allocation Misunderstanding (Code Works -- Need to Correct to Remove Valgrind Error)

Question

My code works fine, but I am receiving valgrind errors. I want to know how to correct my code to in respect to using these malloc and free statements correctly with the char * * dest. Please don't tell me not to malloc and free unless I am doing it at the incorrect locations. Having either or both the corrected code for strcat_ex in answer03.c or an explanation of my misunderstanding of malloc, free, and initialization after malloc would be greatly appreciated. I apologize in advance for the long post, but I wanted to provide everything necessary.

More information: I am mainly focusing on the method strcat_ex (this is not the same as strncat -- read the description of the function to see the difference with int *n). The problem occurs with the fact that I need to remalloc the parameter memory for the string (char *) in dest (char **) and if it doesn't have enough space allocated with it and after I malloc it is not initialized. This doesn't make sense to me how to initialize "heap" memory after a malloc. I didn't believe initialization had to happen after a malloc.

Note: pa03.c and answer03.h should not be changed at all.

Here is the relevant valgrind error (memcheck.log):

==28717== 1 errors in context 7 of 10:
==28717== Conditional jump or move depends on uninitialised value(s)
==28717==    at 0x402D09C: strcat (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==28717==    by 0x8048F98: strcat_ex (answer03.c:29)
==28717==    by 0x8048631: main (pa03.c:16)
==28717==  Uninitialised value was created by a heap allocation
==28717==    at 0x402A17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==28717==    by 0x8048F46: strcat_ex (answer03.c:21)
==28717==    by 0x8048631: main (pa03.c:16)
==28717== 
==28717== ERROR SUMMARY: 10 errors from 10 contexts (suppressed: 0 from 0)

Lines referenced:

Line 16 (from pa03.c) SHOULD NOT BE CHANGE. Serves as an example of a call to the method parameters def, n, src, and return variable result are declared below in pa03.c:

result=strcat_ex(&dest, &n, src);

Line 21 (from answer03.c):

char * buffer = malloc(1 + 2 * (sizeOfDest + strlen(src)));

Line 29 (from answer03.c):

buffer = strcat(buffer,src);

Here is the relevant source code. This is where the valgrind error is and stackoverflow knowledge is needed (answer03.c):

Edit: Comments have been added and lines have been commented out to remove an error of mine in the code that had nothing to do directly with my question. I apologize for these heinous mistakes, but left the lines in there to help future readers understand.

#include "answer03.h"
#include <string.h>

char * strcat_ex(char * * dest, int * n, const char * src)
{
            //Edit: Removed Line Below - Irrelevant variable resplaced with *n
    //int sizeOfDest;

    if(*dest == NULL)
    {
        *n = 0;
    }
    else
    {
        //Edit: Removed Line Below - variable replaced with *n 
        //sizeOfDest = strlen(*dest);
    }
    //Edit: Removed Line Below
    //if(*dest != NULL && sizeOfDest >= 1 + sizeOfDest + strlen(src))
    //Edit: Corrected Line
    if(*dest !=NULL && *n >= 1 + strlen(*dest) + strlen(src)) 
    {
        strcat(*dest, src);
    }
    else
    {
        //Edit: *n replaced sizeOfDest and changes needed to be made to reflect this. Commented out lines were incorrect and irrelevant. Lines directly below them are the corrected versions, until you reach the next blank line

        //*n = 1 + 2 * (sizeOfDest + strlen(src));
        if(*dest != NULL)
           *n = 1 + 2 * (strlen(*dest) + strlen(src));
        else
           *n = 1 + 2 * strlen(src); 

        //char * buffer = malloc(1 + 2 * (sizeOfDest + strlen(src)));
        char * buffer = malloc(sizeof(char) * *n);

        if(*dest != NULL)
        {
            strcpy(buffer, *dest);
            free(*dest);
        }
        *dest = malloc(sizeof(buffer));
        buffer = strcat(buffer,src);
        *dest = buffer;
    }
    return *dest;
}

EVERYTHING BELOW THIS POINT SHOULD REMAIN UNCHANGED AND IS KNOWN TO BE CORRECT:

My compile statement (Makefile):

gcc -Wall -Wshadow -g pa03.c answer03.c -o pa03

My valgrind statement (Makefile):

valgrind --tool=memcheck --leak-check=full --verbose --track-origins=yes --log-file=memcheck.log ./pa03

Here is the function definition for strcat_ex (answer03.h):

#ifndef PA03_H
#define PA03_H 

#include <stdlib.h>

/**
 * Append the C-string 'src' to the end of the C-string '*dest'.
 *
 * strcat_ex(...) will append the C-string 'src' to the end of the string
 * at '*dest'. The parameter 'n' is the address of a int that specifies how
 * many characters can safely be stored in '*dest'. 
 *
 * If '*dest' is NULL, or if '*dest' is not large enough to contain the result
 * (that is, the sum of the lengths of *dest, src, and the null byte), then
 * strcat_ex will:
 * (1) malloc a new buffer of size 1 + 2 * (strlen(*dest) + strlen(src))
 * (2) set '*n' to the size of the new buffer
 * (3) copy '*dest' into the beginning of the new buffer
 * (4) free the memory '*dest', and then set '*dest' to point to the new buffer
 * (5) concatenate 'src' onto the end of '*dest'.
 *
 * Always returns *dest.
 *
 * Why do we need to pass dest as char * *, and n as int *? 
 * Please see the FAQ for an answer.
 *
 * Hint: These <string.h> functions will help: strcat, strcpy, strlen.
 * Hint: Leak no memory.
 */
char * strcat_ex(char * * dest, int * n, const char * src);
//...

Here is the relevant code that calls source as a test (pa03.c):

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

int main(int argc, char **argv)
{
  char * src;
  char * dest;
  char * result;
  int n;

  src="World!";
  dest=NULL;
  result=strcat_ex(&dest, &n, src);
  printf("src=\"World!\";\ndest=NULL;\nstrcat_ex(&dest, &n, src);\n --> gives %s with n=%d\n",result,n);
  result=strcat_ex(&dest, &n, "");
  printf("Then strcat_ex(&dest, &n, \"\") yields --> gives %s with n=%d\n",result,n);
  strcpy(dest,"abc");
  result=strcat_ex(&dest, &n, "def");
  printf("Then strcpy(dest,\"abc\"); strcat_ex(&dest, &n, \"def\") yields --> gives %s with n=%d\n",result,n);  
  free(dest);
  //...

Here is the relevant output (print statments from pa03.c): Note this is correct output (which my current code is able to produce).

src="World!";
dest=NULL;
strcat_ex(&dest, &n, src);
 --> gives World! with n=13
Then strcat_ex(&dest, &n, "") yields --> gives World! with n=13
Then strcpy(dest,"abc"); strcat_ex(&dest, &n, "def") yields --> gives abcdef with n=13
//...

Last words:

I have attached the files needed to compile this code as well as the valgrind error log in linux using gcc and valgrind. There is more in the valgrind, but I posted what I believe to be most relevant. Thanks in advance.

Zip including all files:
http://www.filedropper.com/files_11

Answer 1

Your current function is utterly broken. It contains logic that cannot possible see fruition, at least one memory leak, and unchecked concatenation to a target buffer that is uninitialized. Among the things wrong, which are numerous:

• Assuming sizeofDest dictates not just the storage of the current target string, but also the capacity of any concatenation operation. This is completely wrong, and is why n is provided to this function.
• Outright memory leak: *dest = malloc(sizeof(buffer)); not only allocates a completely incorrect size of memory (the size of a pointer; not what it points to), it summarily leaks said allocation just two lines later.
• Dead-code boolean logic: Given any non-negative value N, the expression N >= N + 1 + M, where M is a non-negative value, is impossible to ever be true.
• You never use the key piece of information provided in tandem with the target pointer-by-address: the current target buffer size provided by n. That value is critical to this algorithm, as it is what dictates the real size of the target buffer, and in conjunction with the current string length in *dest*, will ultimately dictate whether a resize is required.

This is one way to do this function correctly:

char *strcat_ex(char ** dest, int * n, const char * src)
{
    size_t dst_len = 0, src_len = strlen(src);

    // determine current string length held in *dest
    if (*dest && **dest)
        dst_len = strlen(*dest);

    size_t req_len = dst_len + src_len + 1;

    // is space already available for the concatination?
    if (*dest && *n >= req_len)
    {
        // we already know where the target address of the
        // concatination is, and we already know the length of
        // what is being copied. just copy chars.
        if (src_len)
            memcpy(*dest+dst_len, src, src_len+1);
    }
    else
    {
        // resize is required
        void *tmp = realloc(*dest, req_len);
        if (tmp != NULL)
        {
            // resize worked, original content of *dest retained, so
            // we can once again simply copy bytes.
            *dest = tmp;
            memcpy(*dest+dst_len, src, src_len+1);
            *n = (int)req_len;
        }
        else
        {
            perror("Failed to resize target buffer");
            exit(EXIT_FAILURE);
        }
    }

    return *dest;
}

I take great issue with the author of this design in that they chose int for the variable that holds the magnitude of the destination buffer. That magnitude can clearly never be negative, and it makes no sense to use anything other than the same type used by all standard library size-operations, size_t. I'd bring that to the attention of whoever designed this.

Simple Test

int main()
{
    char *dst = NULL;
    int n = 0;

    strcat_ex(&dst, &n, "some string");
    printf("%s : %d\n", dst, n);
    strcat_ex(&dst, &n, " more data");
    printf("%s : %d\n", dst, n);

    *dst = 0; // zero-term the string

    strcat_ex(&dst, &n, "after empty");
    printf("%s : %d\n", dst, n);
    strcat_ex(&dst, &n, " and more");
    printf("%s : %d\n", dst, n);
    strcat_ex(&dst, &n, " and still more");
    printf("%s : %d\n", dst, n);
}

Output

some string : 12
some string more data : 22
after empty : 22
after empty and more : 22
after empty and more and still more : 36

Your Test

Running your test program yields the following:

src="World!";
dest=NULL;
strcat_ex(&dest, &n, src);
 --> gives World! with n=7
Then strcat_ex(&dest, &n, "") yields --> gives World! with n=7
Then strcpy(dest,"abc"); strcat_ex(&dest, &n, "def") yields --> gives abcdef with n=7

Answer 2

If *dest is NULL, you will be calling strcat on buffer when buffer has just been malloc'd and it is uninitialized. Either set buffer[0] to 0, or use calloc.

Also, you allocate *dest and then set *dest to buffer two lines later, which leaks some memory. There is no need for the first assignment to *dest.