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;
}
dbush
  • 205,898
  • 23
  • 218
  • 273
Joe K
  • 55
  • 4
  • 8
    `arr[0] = "string1";` You are re-assigning the pointer. Use [`strcpy()`](https://en.cppreference.com/w/c/string/byte/strcpy) instead. – 001 Jan 17 '19 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 – Craig Estey Jan 17 '19 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. – Et7f3XIV Jan 17 '19 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]. – Craig Estey Jan 17 '19 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**](https://stackoverflow.com/questions/42094465/correctly-allocating-multi-dimensional-arrays). – Andrew Henle Jan 17 '19 at 23:06
  • @Craig Estey my comment about strncpy is for your example you gave (if we don't have limit we can have buffer overflow that is a security breach is like using gets instead of fgets in 2019). And you can see in my answer I free after the first 2 cells (but you are right it is more convenient to free a pointer and store the pointer returned by strdup). I will add +1 for you ^^ – Et7f3XIV Jan 18 '19 at 00:03

5 Answers5

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.

dbush
  • 205,898
  • 23
  • 218
  • 273
  • No. Don't use `strcpy()`. It'll happily overrun your buffers. Use `strdup()` instead. – cmaster - reinstate monica Jan 17 '19 at 21:39
  • 3
    @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. – dbush Jan 17 '19 at 21:57
  • 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. – cmaster - reinstate monica Jan 17 '19 at 22:56
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");
SystemGlitch
  • 2,150
  • 12
  • 27
  • 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. – cmaster - reinstate monica Jan 17 '19 at 21:42
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.

0___________
  • 60,014
  • 4
  • 34
  • 74
  • The third answer that suggest using the extremely dangerous `strcpy()`. The sooner we forget that `strcpy()` ever existed, the better. Use `strdup()` instead. – cmaster - reinstate monica Jan 17 '19 at 21:44
  • 1
    @cmaster it is obviosly not the truth. Do not try to use `strdup` when programming microcontrollers. – 0___________ Jan 17 '19 at 21:45
  • 2
    by the way use strncpy instead of strcpy: to avoid buffer overflow. – Et7f3XIV Jan 17 '19 at 21:54
  • If you use `strncpy` make sure you also explicitly set the last char to a null terminator. https://linux.die.net/man/3/strncpy – Raptor007 Jan 17 '19 at 22:01
  • 2
    strncpy is really dangerous :) – 0___________ Jan 17 '19 at 22:02
  • Ok, `strcpy()` is here to stay for those use cases where we are forced to use it. However, it's a really bad habit to get into to use `strcpy()` wherever it seems to fit. It should only be a last ditch resort, and even then, each use of `strcpy()` requires a written proof in the comments of why its use is legit. Just think about how many hacks have been made possible by the use of `strcpy()` and friends! – cmaster - reinstate monica Jan 17 '19 at 23:00
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;
}
Et7f3XIV
  • 562
  • 1
  • 7
  • 20
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)

Govind Parmar
  • 20,656
  • 7
  • 53
  • 85