Can some tell me why I am seg faulting in this simple C program?

Question

I keep on getting seg faulted after I end my first for loop, and for the life of me I don't why. The file I'm scanning is just 18 strings in 18 lines. I thinks the problem is the way I'm mallocing the double pointer called picks, but I don't know exactly why. I'm am only trying to scanf strings that are less than 15 chars long, so I don't see the problem. Can someone please help.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define MAX_LENGTH 100

int main( int argc,char *argv[] )
{

   char* string = malloc( 15*sizeof(char) );
   char** picks = malloc(15*sizeof(char*));
   FILE* pick_file = fopen( argv[l], "r" );
   int num_picks;


   for( num_picks=0 ; fgets( string, MAX_LENGTH, pick_file ) != NULL ; num_picks++ )
     {
       scanf( "%s", picks+num_picks );
     }
   //this is where i seg fault
   int x;
   for(x=0; x<num_picks;x++)
     printf("s\n", picks+x);
}

Answer 1

1. string is only getting allocated enough memory to store one character (sizeof(char)). If you want to store more characters, you'll need to multiply sizeof(char) by the size of the string you want to store, plus one for the null at the end.

2. Instead of:

   char** picks = malloc(15*sizeof(char));
   

   You want to do this:

   char** picks = malloc(15*sizeof(char*));
   

   Each element of the picks array needs to be big enough to hold a pointer.

Answer 2

First of all, in C strings are stored as byte (char) arrays, and have to be allocated. In this situation, the string used to read he file should be allocated for MAX_LENGTH+1 (+1 for the string terminator, \0) chars:

char* string = malloc( (MAX_LENGTH+1) * sizeof(char) );

This will allocate enough memory for a string of maximum length: MAX_LENGTH.

Another problem is that the array of pointers char **picks is not allocated to store the 18 strings you're expecting to read:

It has to be allocated for 15 char pointers (char *), which also have to be allocated in the first loop.

int main( int argc,char *argv[] )
{
   ...
   char* string = malloc( (MAX_LENGTH+1) * sizeof(char) );
   char** picks = malloc(15*sizeof(char *));
   FILE* pick_file = fopen( argv[l], "r" );
   int num_picks;

   for( num_picks=0 ; fgets( string, MAX_LENGTH, pick_file ) != NULL ; num_picks++ )
     {
       printf("pick a/an %s ", string );
       //--- allocate the char array to store the current string
       picks[num_picks] = malloc (15 * sizeof(char));
       sscanf(string, "%s", picks[num_picks] );
     }

   for(int x=0; x<num_picks;x++)
     printf("%s\n", picks[x]);
}

You also have to test malloc() return value, and you may want to test if the file content really is as expected and does not contain more lines, or lines longer than 15 chars.

---

Also scanf() reads the standard input, I replaced it with sscanf(), and added the missing '%' sign in the second printf().

Answer 3

Is it because in the line:
for( num_picks=0 ; fgets( string, MAX_LENGTH, pick_file ) != NULL ; num_picks++ )
MAX_LENGTH is 100 but you're reading into the variable string, which can only hold an individual character?

Answer 4

• The location pointed to by string is only allocated space for one char, but you try to read up to MAX_LENGTH chars into it;
• The location pointed to by picks is only allocated space for 15 char * pointers, but you apparently want to store pointers to 18 strings there;
• The location pointed to by picks is allocated but never initialised. You need to make those 15 (or 18) pointers actually point to something themselves, before you hand them to scanf.

In this case, there is actually no need for dynamic allocation at all - you could do what you want with arrays:

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

#define MAX_LENGTH 100
#define MAX_LINES 18

int main(int argc, char *argv[])
{
    char string[MAX_LENGTH];
    char picks[MAX_LINES][MAX_LENGTH];
    FILE *pick_file = NULL;
    int num_picks;

    if (argc > 1)
        pick_file = fopen(argv[1], "r");

    if (pick_file == NULL)
        return 1;

    for (num_picks = 0; num_picks < MAX_LINES && fgets(string, MAX_LENGTH, pick_file) != NULL; num_picks++)
    {
        printf("pick a/an %s ", string);
        scanf("%s", picks[num_picks]);
    }

    int x;
    for (x = 0; x < num_picks; x++)
        printf("%s\n", picks[x]);

    return 0;
}

Answer 5

You are allocating an array of fifteen char* for picks:

char** picks = malloc(15*sizeof(char*));

Now picks has enough space to store fifteen char*, but you never actually put any pointers inside that would point to memory that should contain the read characters.

You need to allocate memory for these strings that will be read with scanf. At the moment scanf just writes the data to wherever the pointers in picks (that have not been initialized to point anywhere useful) are pointing to.

Answer 6

u r getting seg fault here.

scanf( "%s", picks+num_picks );

instead do this,

for( num_picks=0 ; fgets( string, MAX_LENGTH, pick_file ) != NULL ; num_picks++ )
 {
   pics[num_picks] = (char* )malloc(100);
   scanf( "%s", picks+num_picks );
 }

The problem is, u allocated **pics to hold 15 strings, but u read the strings without allocating the space for those strings. make sure u always read into allocated pointers.

Answer 7

picks is a pointer-to-a-pointer: that means that the things it points to, are themselves pointers.

When you do this:

char** picks = malloc(15*sizeof(char*));

You are making picks point to a block of 15 pointers - which is fine as far as it goes (although, since you want to read in 18 lines, you really need 18 here instead of 15). This means that picks points to a block of variables in memory like this:

| picks (char **) | --------> | picks[0] (char *)  | ----> ?
                              | picks[1] (char *)  | ----> ?
                              | picks[2] (char *)  | ----> ?
                              | ...                |
                              | picks[14] (char *) | ----> ?

As you can see, those 15 char * pointers are now unintialised - they don't point to anything. You can't start using them until you allocate some memory for those, too - you need to do something like this:

int i;
for (i = 0; i < 15; i++)
{
    picks[i] = malloc(15);
}

Now, after that, the memory layout looks like this:

| picks (char **) | --------> | picks[0] (char *)  | ----> | picks[0][0] (char)  |
                                                           | picks[0][1] (char)  |
                                                           | ...                 |
                                                           | picks[0][14] (char) |

                              | picks[1] (char *)  | ----> | picks[1][0] (char)  |
                                                           | ...                 |

                              | ...                |

...and you now have places to store all those characters you want to read.