0

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.

Improve this question
17
  • When you compiled it segfaulted? Or after you compiled and then ran it? Commented Nov 24, 2017 at 7:02
  • segmentation fault happened after I compiled it and ran it Commented Nov 24, 2017 at 7:03
  • Which line caused the segfault? Commented Nov 24, 2017 at 7:04
  • 6
    @ZCode ask yourself, where does data[i] point to in your fgets line? Commented Nov 24, 2017 at 7:04
  • 1
    @chux yet, had to reevaluate, the pointers are pointers to char * so you need something similar to return strcmp (*(char * const *)a, *(char * const *)b); Additionally, the '{' scope is all mucked up. C doesn't 'allow nested functions. Commented Nov 24, 2017 at 7:07

1 Answer 1

Reset to default
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?

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

If you want to initialize the pointers to NULL anyway, you could alternatively use calloc...
@Aconcagua In practice, yes, but technically, no: calloc allocates all-bits-zero and there are rare platforms where this is not a null pointer (imagine a platform with RAM or ROM mapped at all-bits-zero).
Thank you sir. People have been pointing out that cmpstr which was wrong but an easy fix but they did not realize I was struggling more on the memory aspect of it.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.