CODEWITHSUNDEEP
c
Dracep
Dracep

Reputation: 378

memory leak when concatenating strings into array

I have one function, alloc_str, which takes a string pointer and an array of pointers. It dynamically increases the size of the array by one and appends the string into the array. I have run a GDB debugger and highlighted my memory leak and const error below.

My expected input/output:

array = alloc_str(array, "test_1");
array = alloc_str(array, "test_2");
array = alloc_str(array, "test_3");
--> ["test_1", "test_2", "test_3"]

My alloc_str function:

char **alloc_str(char **existing, const char *add)
{
    int length = 0; //find the length of the array
    for (; existing[length]; length++)
    {
    }
    //allocate memory to copy array array
    char **existing_c = (char **)calloc(length + 2, sizeof(char *));

    for (int i = 0; i < length; i++) //copy original array into new array
    {
        existing_c[i] = existing[i];
    }

    //possible memory leak error
    strncat(existing_c, add, sizeof(existing_c) - strlen(existing_c) - 1);
    existing_c[sizeof(existing_c)-1] = '\0';
    //possible memory leak error
    strncpy(existing, existing_c, sizeof(existing - 1));
    s_copy[sizeof(destsize)-1] = '\0'; //error here

    free(existing);
    return existing_c;
}

void free_array(char **strings) //free's data in array, should be fine
{
    int length = 0;
    for (; strings[length]; length++)
    {
    }
    strings = (char **)calloc(length + 2, sizeof(char *));
}

My main function:

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

int main(){ //should be fine
    char **array = NULL;
    char **test;

    array = (char **)calloc(1, sizeof(char *)); //array has no strings yet

    array = alloc_str(array, "test_1");
    array = alloc_str(array, "test_2");
    array = alloc_str(array, "test_3");


    for (test = array; *test; test++)
    {
        printf("%s\n", *test);
    }

    free_array(array);
}

My error:

Subscript of pointer to function type 'void (const void *, void *, size_t)' (aka 'void (const void *, void *, unsigned long)')

Upvotes: 0

Views: 176

Answers (2)

Gerhardh
Gerhardh

Reputation: 12404

There are multiple problems:

char **alloc_str(char **existing, const char *add)
{
    int length = 0; //find the length of the array
    for (; existing[length]; length++)
    {
    }
    //allocate memory to copy array array
    char **existing_c = (char **)calloc(length + 2, sizeof(char *));

    for (int i = 0; i < length; i++) //copy original array into new array
    {
        existing_c[i] = existing[i];
    }

    ////////////////////////////////////
    //possible memory leak error
    strncat(existing_c, add, sizeof(existing_c) - strlen(existing_c) - 1);
    existing_c[sizeof(existing_c)-1] = '\0';
    //possible memory leak error
    strncpy(existing, existing_c, sizeof(existing - 1));
    s_copy[sizeof(destsize)-1] = '\0'; //error here
    ////////////////////////////////////

    free(existing);
    return existing_c;
}

The part marked with //////////////////////////////////// does not make much sense.

You allocated an array of pointers. Don't treat it like a string. It is no string. Instead simply assign the new pointer to the end of the array and add terminator again.

    existing_c[length] = add;
    existing_c[length+1] = NULL;

With that terminater you could use normal malloc instead of calloc because you assign all elements of the array anyway.

Besides the problem with allocation, you have another memory leak:

void free_array(char **strings) //free's data in array, should be fine
{
    int length = 0;
    for (; strings[length]; length++)
    {
    }
    strings = (char **)calloc(length + 2, sizeof(char *));
}

You pass a pointer to an array of pointers. This array takes some memory that you allocated with calloc earlier. Then you allocate a bit more memory and assign the address to local variable string.

This has two problems:

  1. The memory that was allocated earlier is not freed.
  2. The memory you allocate in this function is not accessible outside of that function.

In the end, your free_array function does not free anything but consumes more memory.

Another problem might be present with the strings that you store in that array. In your example you use string literals. These are static objects and there is no need to free them.

If you will use your functions to store pointers to dynamically allocated string as well, you will need to take care about allocating and freeing the strings as well.

Upvotes: 2

nodakai
nodakai

Reputation: 8033

strncat() works on a memory buffer containing a NUL-terminated (aka "C") string:

char buf[10] = {'a', 'b', 'c', '\0'};
strncat(buf, "def", sizeof(buf) - strlen(buf) - 1);
assert(strcmp(buf, "abcdef") == 0);  // buf now equals to "abcdef"

(Well, the use of strlen() kinda killed the benefit of strncat() over good ol' strcat() but that's another story...)

So it's very different from what you want to do in your exercise. You actually don't need either of strncat() or strncpy().

Upvotes: 1

Related Questions