5

I am using malloc to dynamically allocate a 2d char array. Unless I set every array index to NULL before free(), I get a segmentation fault when trying to free(). Why do I get a segmentation fault?

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

int main(void) {

    int nrows = 20; int ncolumns = 10;
    char ** arr = malloc(nrows * sizeof(char *));
    for(int i = 0; i < nrows; i++)
        arr[i] = malloc(ncolumns * sizeof(char));

    arr[0] = "string1";
    arr[1] = "string2";

    // does not work without the following code:
    // for(int i = 0; i < 20; i++)
    //     arr[i] = NULL;

    for(int i = 0; i < 20; i++)
        free(arr[i]);
    free(arr);

    return 0;
}
6
  • 8
    arr[0] = "string1"; You are re-assigning the pointer. Use strcpy() instead. Commented Jan 17, 2019 at 21:22
  • 1
    Note that [as others have mentioned], you need (e.g.) strcpy(arr[0],"string1"); But, note that "string1" must be no more than 9 chars (i.e. ncolumns - 1). So, be careful. If your second loop did: arr[i] = NULL;, you could do: arr[0] = strdup("string1"); as an alternative. Then, your free loop could do: if (arr[i] != NULL) free(arr[i]); Doing strdup here is usually better because it allows each string in the array to have whatever length it needs vs being constrained to a fixed length. So, it accomodates shorter or longer strings with minimal wasted memory Commented Jan 17, 2019 at 21:31
  • @CraigEstey we donc need that in his case arr[i] point to stack so it != NULL and we can freely free a NULL pinter (it do nothing) maybe you want to store your text in the first arr[0] so use strncpy instead of strcpy (recommended by others): to avoid buffer overflow. Commented Jan 17, 2019 at 21:57
  • 1
    @Et7f3XIV Yes, free(NULL) is harmless. But, it wasn't always so, so sometimes I forget. But, I wouldn't do either strcpy/strncpy but, as I've said, strdup. Otherwise, there's little point to the char **arr. We'd do: char arr[nrows][ncolumns] and strcpy/strncpy. I'm surprised by the suggestions to use strncpy as other responders on other pages have said don't use it [without (e.g.) adding/forcing the EOS terminator after the call]. Commented Jan 17, 2019 at 22:15
  • That's not a 2d array. That's an array of pointers to a bunch of 1d arrays. See Correctly allocating multi-dimensional arrays. Commented Jan 17, 2019 at 23:06

5 Answers 5

3

When you do this:

arr[0] = "string1";
arr[1] = "string2";

You overwrite the contents of arr[0] and arr[1], which contain the addresses of memory returned from malloc, with the address of two string constants. This causes a memory leak, as that memory is no longer accessible. This is also the reason you crash when you call free because these variables no longer hold the addresses of allocated memory.

What you probably want to do here instead is use strcpy, which will copy the contents of the string literal to the memory you allocated.

strcpy(arr[0], "string1");
strcpy(arr[1], "string2");

Now you can free the memory properly.

Sign up to request clarification or add additional context in comments.

3 Comments

No. Don't use strcpy(). It'll happily overrun your buffers. Use strdup() instead.
@cmaster strcpy is fine if you know what you're dealing with, i.e. the source is a string constant and you know the size of the destination. strdup may not be appropriate based on the situation, and with strncpy you need to manually add the null byte at the end if the source is too big.
Which is precisely what the OP does not seem to know. I know I can use strcpy() relatively safely. But that ability comes only at the price of never wanting to use it, unless forced. If you still want to use strcpy() over strdup(), you have simply not learned the lesson yet.
3

Your code is ok, the problem comes from the fact that you are assigning string literals to your array here: arr[0] = "string1";.

You are thus replacing the pointer at arr[0], which is pointing to your allocated memory, with the pointer to a string literal.

Pointers to literals are protected, you cannot free (nor write to them) them because you didn't allocate them.

To solve this problem, use strcpy to copy the value of your literal inside your allocated memory:

strcpy(arr[0], "string1");
strcpy(arr[1], "string2");

1 Comment

Same issue as with dbush's answer: strcpy() is very dangerous, and shouldn't be used without proof of correct usage. Use strdup() instead, and you'll save yourself from a lot of headaches.
1

= operator does not copy the string it only assigns the pointer. So your malloced memory is not accessible anymore for those array elements and attempt to free it is an Undefined Behavoiur which may lead to the segfault.

You need to copy it using strcpy.

6 Comments

The third answer that suggest using the extremely dangerous strcpy(). The sooner we forget that strcpy() ever existed, the better. Use strdup() instead.
@cmaster it is obviosly not the truth. Do not try to use strdup when programming microcontrollers.
by the way use strncpy instead of strcpy: to avoid buffer overflow.
If you use strncpy make sure you also explicitly set the last char to a null terminator. linux.die.net/man/3/strncpy
strncpy is really dangerous :)
|
1

Crashing upon a call to free is a sign of incorrect memory management somewhere else in your code. When you set a pointer to NULL then free it, you are not going to crash, because free(NULL) is guaranteed to be benign by the C Standard § 7.22.3.3:

7.22.3.3 The free function

... If ptr is a null pointer, no action occurs. Otherwise, if the argument does not match a pointer earlier returned by a memory management function, or if the space has been deallocated by a call to free or realloc, the behavior is undefined.

Emphasis mine.

As other answers have noted, you are trying to call free on memory that you didn't explicitly allocate with malloc-family functions (since you overwrote arr[i] pointers with pointers to string literals)

Comments

1

Two things to know:

  • You have two area in you memory (to make easy t understand) heap and stack
  • malloc, realloc, calloc allocate ressource from heap. I will say only malloc (but it is the same)
    • free can only free ressource from heap. Stack is reserver for the compiler (it store function call and other data)

The rule for each ressource you get from malloc you have to free it. to free simply call the free function (but we can optionally assigne null pointer to be sure it is freed).

char * a = malloc(255);

to free

free(a);/* this is mandatory */
a = NULL;/* we can add this to the first line */

In fact it you take the habit to assign NULL value and one time you access it's value you will have NULL deference error: so you will know where to find error

What you try to do: alloc a array char ** arr = malloc(nrows * sizeof(char *)); and you free it free(arr);

but you alloc 20 arrays of char arr[i] = malloc(ncolumns * sizeof(char)); you ignore it's value arr[0] = "string1"; (you loose the value returned by malloc so you can't free now arr[0]) we are not in C++. So "string1" is stocked on the stack (so malloc can't free it) and you call free on it.

what you can do

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

int main(void) {

    int nrows = 20; int ncolumns = 10;
    char ** arr = malloc(nrows * sizeof(char *));
    for(int i = 0; i < nrows; i++)
        arr[i] = malloc(ncolumns * sizeof(char));
    free(arr[0]);//know we can loose it value because it is freed
    arr[0] = NULL;// in fact we assign a value just after so this line is useless but is educationnal purpose
    free(arr[1]);//know we can loose it value because it is freed
    arr[1] = NULL;// in fact we assign a value just after so this line is useless but is educationnal purpose
    arr[0] = "string1";
    arr[1] = "string2";

    // does not work without the following code:
    // for(int i = 0; i < 20; i++)
    //     arr[i] = NULL;

    for(int i = 2; i < 20; i++)//we start at 2 because the first two value are on the stack
    {
        free(arr[i]);
        arr[i] = NULL;//this is useless because we will free arr just after the loop)
    }
    free(arr);
    arr = NULL;// this is useless because we exit the end of program

    return 0;
}

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.