Getting seg fault from malloc passed double pointer. Confused about syntax

Question

I am only ~2 weeks into an intro to C course so I'm still in the process of comprehending a lot of this. I may not understand technical answers but I do appreciate any insight given. Also, we haven't learned much content yet so advanced techniques will be way over my head.

I am writing a program that will read from a text file and store characters into a double pointer "array" (for lack of a better term). It's basically storing a MxM word search puzzle.

The puzzle will never be larger than 50x50 so I start by malloc the first row to 50 characters, store the first row to determine the number of columns so I can realloc the number of "rows and columns" to the proper size and store the rest of the puzzle.

I am getting a segmentation fault after the do/while is on its second iteration. I think my syntax may be off either of my first two malloc commands (the use of a passed double pointer is confusing me). Also, I know I haven't yet added code to handle a failed malloc.

If anyone can guide me in the right direction I would be most appreciative. Thanks in advance.

This is what I have so far:

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

void storePuzzle(char***);

int main()
{
    char **arr;

    storePuzzle(&arr);
    return 0;
}

void storePuzzle(char ***arr)
{
    int count, count2, size = 0;
    int c;

    *arr = (char**)malloc(sizeof(char*));
    *arr[0] = (char*)malloc(sizeof(char)*50);

    /*determine size of wordsearch and fill first row*/
    do
    {
        c = getchar();
        if(isalpha(c))
        {
            *arr[0][size] = c;
            size++;
        }
    } while(c  != '\n');

    /*realloc row/columns*/
    *arr = (char**)realloc(*arr, sizeof(char*)*size);

    for(count = 0; count < size; count++)
    {   
        *arr[count] = (char*)realloc(*arr[count], sizeof(char)*size);
    }
}

Answer 1

Ok, here are some tips:

char **arr;

storePuzzle(&arr);
return 0;

In the above code block, you don't initialize arr and you don't use the result afterwards. Therefore, there is no reason for this parameter to even exist. If you intend to use the value afterwards, you can just have storePuzzle return the pointer to you.

*arr[0]

[] has higher precedence than *. Also *arr and arr[0] do basically the same thing, so this kind of works when you use 0, but not any other number.

*arr[count] = (char*)realloc(*arr[count], sizeof(char)*size);

You are reallocating pointers that have never been allocated. Remember that you allocated memory for arr[0], but none of the others. If you want to take this approach, you can realloc (*arr)[0], but the others need to use malloc:

(*arr)[count] = (char*)malloc(sizeof(char)*size);

Answer 2

The following will guide on some of the allocating issues, yet is not a fix for all OP's code.

---

When allocating memory, consider the pattern:

object_pointer = /* no cast needed */ malloc(sizeof *object_pointer * number_needed);
if (object_pointer == NULL) OutOfMemory();

When re-allocating:

void_pointer = /* no cast needed */ realloc(object_pointer, 
    sizeof *object_pointer * new_number_needed);
if (void_pointer == NULL) {
  Handle Out of memory,  original object_pointer and its size still valid.
  maybe exit with error message
} 
object_pointer = /* cast not needed */ void_pointer 

How does this affect storePuzzle(char ***arr)?

// Avoid magic numbers like 50 littering code
#define PUZZLE_N 50

void storePuzzle(char ***arr) {
    *arr = malloc(sizeof *(*arr));
    if (*arr == NULL) return;

    *arr[0] = malloc(sizeof *(*arr[0]) * PUZZLE_N);
    if (*arr[0] == NULL) {
      free(*arr);
      return;
    }

    /*determine size of wordsearch and fill first row*/
    size_t size = 0;
    int c;
    do {
        c = getchar();
        if (isalpha(c)) {
            *arr[0][size] = c;
            size++;
        }
    } while(c != '\n' && c != EOF && size < PUZZLE_N);

    // Did you want a null character at the end?
    *arr[0][size++] = '\0';

    /* realloc row/columns */
    void *p = realloc(*arr, sizeof *(*arr) * size);
    if (p == NULL) Handle_OOM(); // details omitted
    *arr = p;

    for(size_t count = 0; count < size; count++) {   
      void *p = realloc(*arr[count], sizeof *(*arr[count]) * size);
      if (p == NULL) Handle_OOM(); // details omitted
      *arr[count] = p;
    }
}

Answer 3

Consider consolidating some of your functionality into smaller modules, that can more easily be debugged in the event you are having difficulty. Such as when you need to create an array of buffers (i.e. char **array), dedicate one function to do that. For example:

char ** Create2D(int lines, int lengths)
{
    int i;
    char **arr = NULL;
    arr = calloc(lines, sizeof(*arr));//creates lines pointers, one for each row of data.
    if(arr)
    {
        for(i=0;i<lines;i++)
        {
            arr[i] = calloc(lengths, 1);//creates space for content in each row.
        }
    }
    return arr;
}

Notice in the calls to calloc, the return is not cast as this is not recommended in C.
Using the same approach as shown here (and going one level deeper) a function could be written to accommodate a triple pointer, char ***array, although with some care, your program can probably be written so that is not required. See these comments on being a three star programmer.

This 2D function would be called like this:

int main(void)
{
    int i;
    char **arr = Create2D(10, 80);//creates 10 pointers each having room for 80 characters
    if(arr)
    {
        storePuzzle(&arr);//pre-allocated buffer should simplify storePuzzle function
        Free2D(arr, 10); //defined below, frees all memory allocated above.
    }
    return 0;
}

And for every dynamic memory allocation, there needs to be a corresponding free called:

void Free2D(char **arr, int lines)
{
    int i;
    for(i=0;i<lines;i++)
    {
        free(arr[i]);
    }
    free(arr);
}

Answer 4

First pointers and arrays are two completely separate types in C. While you can access values stored in an array with pointers and you can model a 2D array with a pointer to pointer to type, the pointer to pointer to type is NOT an array itself.

Arrays are declared, and only declared with brackets, e.g. type name[x];. Here name is a single dimensional array with x elements of type type. (e.g. for x integers it would be int name[x];. (a discussion of compound literal initialization is left for later)

You can also define a pointer to a block of memory that holds x integers, e.g. int *namep = name; where the integer pointer namep now points to the address of the beginning element in name.

You can also allocate that block of memory on your own to create storage for x ints with, e.g. int *namep = malloc (x * sizeof (int)); (which the preferred equivalent would be int *namep = (x * sizeof *namep);) Here namep points to the beginning address of a block of memory sufficiently sized to hold x ints, just like int name[x] does, but there are important differences.

sizeof (an array type) (e.g. sizeof (name) returns the number of bytes in the array. However sizeof (a pointer) (e.g. sizeof (namep) -- or just sizeof namep) returns just that -- the sizeof a pointer -- which is generally 4-bytes on x86 or 8-bytes on x86_64.

When passed as a parameter to a function, the first level of array indirection is converted to a pointer. For example, if you pass name as a parameter to a function, e.g. the function

void print_array (int *array, size_t size)
{
    size_t i;
    for (i = 0; i < size; i++)
        printf ("array[%2zu] : %d\n", i, array[i]);
}

You would pass name to the function as print_array (name, x). Here the array name[x] is automatically converted to a pointer to its first element when passed to print_array. Since namep is already a pointer, no conversion takes place, so you would simply have print_array (namep, x) with no conversion taking place. Why is the conversion important?

What would happen if you modified your function as follows and passed name to it?:

void print_array (int *array)
{
    int i,
        n = (int)(sizeof array / sizeof *array);

    for (i = 0; i < n; i++)
        printf ("array[%2zu] : %d\n", i, array[i]);
}

Would n actually hold the number of elements? (Answer: No). Recall, when the array name was passed as a parameter, it was converted to a pointer. So in this incorrect version sizeof array is just sizeof (a pointer) and sizeof *array is just sizeof (int) where there resulting quotient would be 1 on x86 (1/1) or 2 on x86_64 (2/1).

Now let's turn to your code - why you are starting with a pointer to pointer to char instead of a 2D array is bewildering, but nothing wrong with it. However, rather than becomming a three-star programmer (generally not a complement), let's utilize the function return type to do the same thing and avoid the dubious distinction. We will follow with a simple 2D array example next. (Note: there are intentional changes from walking the input buffer with a pointer in the example below, and using array indexes in the 2D array example that follows to demonstrate that using both pointer arithmetic or array indexing are equivalent)

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

#define ARSZ 50     /* don't use magic numbers in code, define constants */

char **storepuzzle (int row, int col);

int main (void) {

    char **arr = NULL;

    if (!(arr = storepuzzle (ARSZ, ARSZ))) {  /* validate successful fill */
        fprintf (stderr, "error: storepuzzle failure.\n");
        return 1;
    }

    for (int i = 0; i < ARSZ; i++) {
        for (int j = 0; j < ARSZ; j++)
            printf (" %c", arr[i][j]);
        putchar ('\n');
        free (arr[i]);  /* don't forget to free rows when done with them */
    }

    free (arr);         /* free pointers */

    return 0;
}

char **storepuzzle (int row, int col)
{
    char **tmp = NULL,
        buf[ARSZ + 2] = ""; /* buf to read each line into */
    int ridx = 0;           /* row index */

    tmp = calloc (row, sizeof *tmp);    /* allocate row pointers to char * */
    if (!tmp) {
        fprintf (stderr, "error: memory exhausted.\n");
        return NULL;
    }

    while (fgets (buf, sizeof buf, stdin))
    {
        char *p = buf;      /* pointer to 1st char in buf */
        int cidx = 0;       /* column index */
        tmp[ridx] = calloc (col, sizeof *tmp[ridx]);    /* allocate col chars for row */

        if (!tmp[ridx]) {
            fprintf (stderr, "error: memory exhausted at tmp[%d].\n", ridx);
            exit (EXIT_FAILURE);
        }

        while (*p && *p != '\n')    /* copy each char to column */
            tmp[ridx][cidx++] = *p++;

        if (cidx != col)    /* validate col columns filled */
            fprintf (stderr, "warning: insuffient input for row[%d].\n", ridx);

        ridx++;
    }

    if (ridx != row) {  /* validate row rows filled */
        fprintf (stderr, "error: insufficient number of rows filled.\n");
        for (int i = 0; i < ridx; i++)
            free (tmp[i]);
        free (tmp);
    }

    return tmp;
}

Example Input File

50 lines of 50 random characters:

$ cat dat/arr50x50.txt
jjnicknlocbvgnpzfbvbbwlfvoobyjqhkehmoupvprqvwfmcga
vhwheknsldtukdykpmefhlopgkulealszzzvjennighkjfuzjr
<snip>
hfcbxnhqooijevomkwzbudzbdwtsfimnooodbnuitcryqxkauj
ugethhibrnbeahkolebfmvhvlxsnqewklavkzddjrxfjepqptr

Example Use/Output

$ ./bin/arr50x50 <dat/arr50x50.txt
 j j n i c k n l o c b v g n p z f b v b b w l f v o o b y j q h k e h m o u p v p r q v w f m c g a
 v h w h e k n s l d t u k d y k p m e f h l o p g k u l e a l s z z z v j e n n i g h k j f u z j r
<snip>
 h f c b x n h q o o i j e v o m k w z b u d z b d w t s f i m n o o o d b n u i t c r y q x k a u j
 u g e t h h i b r n b e a h k o l e b f m v h v l x s n q e w k l a v k z d d j r x f j e p q p t r

Memory Use/Error Check

In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.

It is imperative that you use a memory error checking program to insure you do not attempt to write beyond/outside the bounds of your allocated block of memory, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.

For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.

$ valgrind ./bin/arr50x50 <dat/arr50x50.txt
==21813== Memcheck, a memory error detector
==21813== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==21813== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==21813== Command: ./bin/arr50x50
==21813==
 j j n i c k n l o c b v g n p z f b v b b w l f v o o b y j q h k e h m o u p v p r q v w f m c g a
 v h w h e k n s l d t u k d y k p m e f h l o p g k u l e a l s z z z v j e n n i g h k j f u z j r
<snip>
 h f c b x n h q o o i j e v o m k w z b u d z b d w t s f i m n o o o d b n u i t c r y q x k a u j
 u g e t h h i b r n b e a h k o l e b f m v h v l x s n q e w k l a v k z d d j r x f j e p q p t r
==21813==
==21813== HEAP SUMMARY:
==21813==     in use at exit: 0 bytes in 0 blocks
==21813==   total heap usage: 51 allocs, 51 frees, 2,900 bytes allocated
==21813==
==21813== All heap blocks were freed -- no leaks are possible
==21813==
==21813== For counts of detected and suppressed errors, rerun with: -v
==21813== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm that you have freed all memory you have allocated and that there are no memory errors.

---

Using a Statically Declared 2D Array Instead

Here, rather than returning a pointer to indicate success/failure, you can simply return an integer value where 0 indicates failure and any other value indicates success, e.g.

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

#define ARSZ 50     /* don't use magic numbers in code, define constants */

int storepuzzle (char (*array)[ARSZ], int row, int col);

int main (void) {

    char arr[ARSZ][ARSZ] = {{0}};   /* actual 2D array initialized to zeros */

    if (!(storepuzzle (arr, ARSZ, ARSZ))) {  /* validate successful fill */
        fprintf (stderr, "error: storepuzzle failure.\n");
        return 1;
    }

    for (int i = 0; i < ARSZ; i++) {
        for (int j = 0; j < ARSZ; j++)
            printf (" %c", arr[i][j]);
        putchar ('\n');
    }

    return 0;
}

int storepuzzle (char (*array)[ARSZ], int row, int col)
{
    char buf[ARSZ + 2] = ""; /* buf to read each line into */
    int ridx = 0;           /* row index */

    while (fgets (buf, sizeof buf, stdin))
    {
        int cidx = 0;       /* column index */

        for (int i = 0; buf[i] && buf[i] != '\n'; i++, cidx++)
            array[ridx][cidx] = buf[i];   /* copy each char to column */

        if (cidx != col) {  /* validate col columns filled */
            fprintf (stderr, "warning: insuffient input for row[%d].\n", ridx);
            return 0;       /* return 0, indicating failure */
        }

        ridx++;
    }

    if (ridx != row) {  /* validate row rows filled */
        fprintf (stderr, "error: insufficient number of rows filled.\n");
        return 0;       /* return failure */
    }

    return ridx;
}

Use/Output Identical - No Need to Validate Memory Use

Unless you need or want to dynamically declare a pointer to pointer to char, using a 2D array simplifies things. You can mix the two for a dynamic allocation by declaring arr as a pointer to array of char [50], but that is for another day...

Look things over, understand both methods, and let me know if you have any questions. Your original sin was failing to appreciate C-Operator precedence with *arr[0][size] = c; instead of (*arr)[0][size] = c;, but since you want to avoid being a 3-star programmer here, it is left to you as an exercise.