2

I am converting integers to strings and adding them to a dynamically allocated array. The problem is that it is causing a segfault. I don't understand why it is happening.

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

int main() {
    char *x = malloc(10 * sizeof(char));
    x[0] = malloc(10 * sizeof(char));
    sprintf(x[0],"%d",10);
    
    for(int i = 0; i < 10;i++){
        free(x[i]);
    }
    
    free(x);
    return 0;
}
2
  • 2
    I assume you are trying to create an array of c-strings. However, your array x is only an array of chars. If this assumption is correct, you want to declare x as char **x and initialize it with malloc(10 * sizeof(char*)) Commented May 3, 2021 at 9:08
  • 1
    Here is a quick example to demonstrate the differences Commented May 3, 2021 at 9:15

3 Answers 3

3

To allocate an array whose elements are char*, the pointer to point the array should be char**, not char*.

Also you mustn't use values in buffer allocated via malloc() and not initiaized. The values are indeterminate and using them invokes undefined behavior.

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

int main() {
    /* correct type here (both variable and allocation size) */
    char **x = malloc(10 * sizeof(char*));
    x[0] = malloc(10 * sizeof(char));
    sprintf(x[0],"%d",10);

    /* initialize the other elements to pass to free() */
    for (int i = 1; i < 10; i++) x[i] = NULL;
    
    for(int i = 0; i < 10;i++){
        free(x[i]);
    }
    
    free(x);
    return 0;
}
Sign up to request clarification or add additional context in comments.

4 Comments

This is correct answer, but personally I would prefer to set all pointers on array to NULL first, before any of the strings are allocated.
@user694733 if you'd like to set all pointers to NULL (which is basically 0) you can use calloc instead of malloc (note that the definition of NULL is implementation detail, and is therefore free for the compiler to set, meaning, it is possible that NULL is not 0, but in most cases it will)
@gkhaos, despite the value NULL could have, it is warranted (or should be) by the implementation that the value it gets is representing a pointer with an invalid reference. This means you cannot derreference a NULL pointer. If you call calloc() to initialize an array of pointers, it should be warranted that the values after initialization are a set of NULL pointers, so there's no difference in initializing them to NULL or using the result of the block initialized by calloc(). And this is the main reason for NULL to be 0 in almost any known implementation.
It is safer to always use snprintf() instead of sprintf().
2

If you want a dynamic allocated array of strings, you should declare your variable x as a pointer to an array of e.g. 32 chars. The you can allocate/deallocate an array of these using a single malloc and likewise a single free.

Like:

#define NUM_STRINGS 10
#define STRING_SIZE 32

int main() {
  // declare x as a pointer to an array of STRING_SIZE chars
  char (*x)[STRING_SIZE];

  // Allocate space for NUM_STRINGS of the above array, i.e.
  // allocate an array with NUM_STRINGS arrays of STRING_SIZE chars
  x = malloc(NUM_STRINGS * sizeof *x);
  if (x)
  {
    for (int i = 0; i < NUM_STRINGS; ++i)
    {
      sprintf(x[i], "%d", 10 + i);
    }

    for (int i = 0; i < NUM_STRINGS; ++i)
    {
      puts(x[i]);
    }

    free(x);
  }

  return 0;
}

Output:

10
11
12
13
14
15
16
17
18
19

Comments

0

The best way to determine the amount of memory to be used with malloc is this:

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

#define N_STRINGS 10
#define STRING_SZ 10

int main() {
    // if you use *x (the deferred subexpression) the compiler can calculate its
    // sizeof easily, and no need to use a constant or something that has to be
    // revised if you change the type of x.  Also, calloc will give instead N_STRINGS
    // pointers already initialized to NULL.
    char **x = calloc(N_STRINGS, sizeof *x);

    // to be able to free(x[i]) for all i, you need to initialize all pointers,
    // and not only the first one.
    int i; // I prefer this, instead of for(int i..., which is more portable with legacy code.
    for (i = 0; i < N_STRINGS; i++) {
        // char is warranted to be sizeof 1, you don't need to specify but the
        // number of chars you want for each character array.
        x[i] = malloc(STRING_SZ); // remember only STRING_SZ chars you have, so...
        // better to use snprintf(), instead.
        snprintf(x[i], // the buffer pointer
                 STRING_SZ,   // the buffer size (STRING_SZ chars, incl. the final null char)
                 "%d", // the format string
                 10);  // initialize all strings to the string "10" ???
    }
    // upto here we have made N_STRINGS + 1 calls to malloc...

    // you should do something here to check that the allocations went fine, like
    // printing the values or do some work on the strings, but that's up to you.
    
    // now, everything should be fine for free() to work.
    for(int i = 0; i < N_STRINGS; i++){
        free(x[i]);
    }
    
    free(x); // ... and this is the N_STRINGS + 1 call to free.
    return 0;
}

Check always that the number of free calls executed by your program has to be the same of the malloc calls you have made (before). A free call must free the memory allocated by one (and only one) call to malloc. You cannot free something that has not been acquired by malloc() (or calloc()) directly or indirectly. The same as it is bad use (but not necessary an error) to do a malloc that is not freed before the program ends (this is not true in non-hosted environments, e.g. embedded programs, in which the operating system doesn't deallocate the memory used by a program when it finishes, although)

By the way, the reason of your segmentation fault is precisely that you have made only two calls to malloc(), but you made 11 calls to free(). free() tries to free memory that malloc() has not allocated, or even worse, you don't own. Anyway, this drives you to Undefined Behaviour, which is something you don't desire in a program. In this case, you got a program crash.

4 Comments

to be able to free(x[i]) for all i, you need to initialize all pointers, and not only the first one... This is incorrect: it is OK to call free() with a null pointer. The C Standard mandates free() to handle this special case gracefully and do nothing.
i was not defined at all at the function scope and defined in the local scope of the second for loop... I merely added the definition in the first loop for consistency with your choice in the second loop. I guess I should have just commented...
... which I did eventually after re-reading the code comments.
Yes, you are right... I was seeing code you have not yet seen. Thanks anyway!

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.