Sorting strings in a program - C

Question

So I'm trying to make a program that takes in certain number of strings from stdin and then output into a text file. The code I have so far is:

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

int cmpstr(const void * a, const void *b){
    const char* aa = (const char*) a;
    const char* bb = (const char*) b;
    return strcmp (aa, bb);
}

int main(int argc, char *argv[]){
  int i =0;
  int scount;
  char ** data = NULL;
  FILE * ofile;




if (argc != 3){
printf("%s \n", "The format to use this is: mySort <#of strings> <output filename>",argv[0]);
exit(EXIT_FAILURE);
}

scount = atoi(argv[1]);
if(!scount){
printf("%s \n", "Invalid number.");
exit(EXIT_FAILURE);
}

data = (char **) malloc(scount * sizeof(char*));
if(NULL == data){   
printf("Memory allocation failed\n");
exit(EXIT_FAILURE);
}

for(i = 0; i< scount; i++){
if(NULL == fgets(data[i], (int) sizeof(data), stdin)){
printf("Could not get line\n");
exit(EXIT_FAILURE);
}
}

qsort(data, scount, sizeof(char*), cmpstr);

ofile = fopen(argv[2], "w+");
if(ofile == NULL){
printf("Could not open output file. \n");
}   

for(i = 0; i<scount; i++){
    fputs(data[i], ofile);
}

fclose(ofile);

for(i=0; i<scount; i++){
    if(data[i]) free(data[i]);
}

if (data) free(data);

exit (EXIT_SUCCESS);

return 0;
}

However, when I compiled it it gave me a segmentation fault. I tried using the gdb debugger to try and debug it but it did not give me anything really and I barely understand how to use gdb. But my takeaway from the usage of gdb is that there is not enough memory allocated which confuses me since I allocated memory using malloc.

Answer 1

data = (char **) malloc(scount * sizeof(char*));

Here you allocate memory for an array of pointers. You never initialize the contents of that array. Therefore, when you access data[0] below by passing it to fgets, you're accessing an uninitialized pointer object. If you're lucky, the contents of that uninitialized memory constitutes an invalid address, and when fgets attempts to store data there, your program crashes. If you're unlucky, the contents of that uninitialized memory happens to be the address of some block of memory that's used by some other object and you get memory corruption that's really hard to debug.

You need to initialize the pointers allocated by malloc. Like any other pointer object, depending on what you want to do, you can initialize them to NULL, to a pointer to an existing object, or to the result of calling a function such as malloc. In this program, you need to obtain storage for the lines that you're going to read, therefore you'll need to call malloc on each of the lines. Since you don't know in advance how long a line will be, this is best done at the time you read the line.

It would be a good idea to first set all the elements to NULL, as soon as you've allocated the array of pointers, and later allocate memory for the individual lines. You don't have to, but it's easier then to keep track of which elements of the array have been initialized and which ones haven't. In particular, that lets you call free on all the array elements without having to worry how many you've already reached.

fgets(data[i], (int) sizeof(data), stdin)

Passing sizeof(data) here doesn't make sense. The variable data is a pointer to char*, so sizeof(data) is just the size of a pointer. It isn't the size of the array that the pointer points to: that size isn't known at compile time, it's the argument you pass to malloc. And even that size is not relevant here: the size is the maximum number of lines you can read (multiplied by the size of a pointer to a line's contents), but what fgets needs is the size of the memory that's allocated for the line.

To keep things simple, let's say you have a maximum line length max_line_length.

data = (char **) malloc(scount * sizeof(char*));
if (data == NULL) ... // omitted error checking
for (i = 0; i < scount; i++)
    data[i] = NULL;
for (i = 0; i < scount; i++) {
    data[i] = malloc(max_line_length+2); // +2 for line break character and null byte to terminate the string
    if (data[i] == NULL) ... // omitted error checking
    if(NULL == fgets(data[i], max_line_length, stdin)) ... // omitted error checking
    ...
}

After this you'll run into another issue as described in the comments, in that cmpstr receives pointers to pointers to line contents, not pointers to line contents. This is explained in How to qsort an array of pointers to char in C?