1

I'm trying to write a stream editor in C and I'm having a hard time dealing with strings. After reading in the lines of a File, I want to store them locally in an array of Strings. However, when I try to store the variable temp into the array of strings StoredEdits I get a segmentation fault (core dumped) error. Furthermore, if I uncomment the char* temp2 variable and save this into my array as a workaround, then the last value read in gets stored for every value in the array.

I assume this has to do with the fact that temp2 is a pointer. I've tried a million things like malloc'ing and free'ing this variable after each iteration, but nothing seems to work.

Any help would be greatly appreciated.

      #define MAX_SIZE 100 
      typedef char String[MAX_SIZE];   
          int main(int argc, char* argv[]){
              char** StoredEdits;                                                                                                                                                                    
              int index, numOfEdits;
              FILE *EditFile;                                                                                                                                                                                       
              char* temp;
              //char* temp2;                                                                                                                                                                                            

              StoredEdits = (char**)malloc(MAX_INPUT_SIZE*sizeof(String));                                                                                                                                           

              /*Check to see that edit file is passed in.*/
              if(argc < 2){
                printf("ERROR: Edit File not given\n");
                return(EXIT_FAILURE);
              }

              printf("%s\n",argv[1]);

              if( (EditFile = fopen(argv[1],"r")) != NULL ){
                printf("file opened\n");
                numOfEdits = 0;
                while(fgets(temp, MAX_STRING_SIZE, EditFile) != NULL){
                  printf("%d %s",numOfEdits,temp);
                  //temp2 = temp;                                                                                                                                                                                       
                  StoredEdits[numOfEdits++] = temp;
                  //StoredEdits[numOfEdits++] = temp;
                  printf("Stored successfully\n");
                }

..........

              printf("%d\n",numOfEdits);
              for(index=0;index<numOfEdits;index++){
                printf("%d %s\n",index, StoredEdits[index]);
              }
aensm
  • 3,325
  • 9
  • 34
  • 44
  • Please [don't cast the return value of `malloc()` in C](http://stackoverflow.com/a/605858/28169). – unwind Apr 08 '13 at 08:26

3 Answers3

4

You need to initialize temp to point to valid storage.

temp = malloc(MAX_STRING_SIZE+1);

It looks like you may have intended to do something like this:

String temp;

using your macro. This would be better as a regular char array. And the common name for this is buffer.

char buffer[MAX_STRING_SIZE+1];

Then, you should store in your array, not temp itself, but a new string containing a copy of the contents. There is a POSIX function strdup that should be helpful here. Note, strdup is not part of the C standard, but it is available in most hosted implementations. Historically, it comes from the BSD branch.

StoredEdits[numOfEdits++] = strdup(temp);

Let me backpedal a little and say that if you're allocating new storage for temp inside the loop, then you should skip the strdup because, as Jim Balter says, this will leak memory. If you allocate temp outside of the loop, then it makes little difference whether you allocate it statically (by declaring a char []) or dynamically (with malloc).

By the way, this line will not buy you much:

typedef char String[MAX_SIZE];

For why, see the classic Kernighan (the K in K&R) essay Why Pascal is not my favorite Programming Language.

Also note, that my examples above do not check the pointer returned by malloc. malloc can fail. When malloc fails it will return a NULL pointer. If you try to store data through this pointer, Kaboom!

luser droog
  • 18,988
  • 3
  • 53
  • 105
  • if you both set `temp` to the result of `malloc` and `strdup` it you'll leak memory. Either use `malloc` and skip the `strdup`, or (better, I think, because it doesn't waste memory), use a buffer on the stack and `strdup` it. – Jim Balter Apr 08 '13 at 05:51
  • @JimBalter would it still leak if I free(temp) after each iteration? – aensm Apr 08 '13 at 05:57
  • @aensm That would fix the leak, but then `strdup` is largely superfluous. It would have the side-effect of shrinking the storage to exactly fit the string. If you've got a gazillion of them, that might be important. – luser droog Apr 08 '13 at 06:16
3

You're right about your problem being because of pointer semantics. You should use copy the contents of the string from temp.

char *cpy = malloc(1 + strlen(temp));
if (cpy)
    strcpy(cpy, temp);
//else handle error
StoredEdits[numOfEdits++] = cpy;
AlexanderBrevig
  • 1,967
  • 12
  • 17
0

Others answered the reason for the error.

But from the program, i see that you tried to allocate a character double array. then you store each line read from the file into the array.

StoredEdits = (char**)malloc(MAX_INPUT_SIZE*sizeof(String));

if my assumption is right, then you should pass the array into strcpy like the below.

strcpy(StoredEdits[numOfEdits],tmp);

when you have a file where each line varies in size, it is better to go array of pointers points to character array.

nvseenu
  • 125
  • 6