strcpy char pointer abort

Question

I have this code:

     char **arr;
     char* line=NULL;
     int i=0;
     size_t len=0;
     ssize_t read1;

     fp=fopen("list.txt","r");
     if(fp==NULL)
         exit(EXIT_FAILURE);

     while((read1=getline(&line,&len,fp))!=-1)
         i++;
     fclose(fp);

     fp=fopen("list.txt","r");
     if(fp==NULL)
         exit(EXIT_FAILURE);

     arr=(char**)malloc(i*sizeof(char*)); // i is a variable i use to know the number of lines
     i=0;

     while((read1=getline(&line,&len,fp))!=-1)
     {
         line[strlen(line)]='\0';
         arr[i]=(char*)malloc(strlen(line)+1);
         strcpy(arr[i],line);
         i++;
     }

When I try strcpy the program crashes.Is a malloc problem? I am very sure that i is big enough. And the line is char* and is NULL at first.

EDIT: I forgot that this program is in Qt.

You don't need to cast the return value of `malloc()` in a C program. — Carl Norum, Jan 24 '13 at 19:04
***"`i` is a variable i use to know the number of lines"*** - Then why is it named `i` and not `numberOfLines` / `linesNumber` ? — LihO, Jan 24 '13 at 19:05
How is `line` defined? And what is is `len`'s definition/value? — Evan Teran, Jan 24 '13 at 19:05
@Oil: indeed, good catch, @Emil, what is the value of `i` before this code? — Evan Teran, Jan 24 '13 at 19:08
@Emil: since you define `line` to be `NULL`, that means you expect `getline` to allocate the buffer for you. So a couple of things. 1) You don't need `strcpy` then, you can just store the pointer, it's yours to manage. 2) you need to set line to `NULL` after you are done with it, otherwise the **next** call will reuse that buffer, possibly being too short and overflowing if it is a longer line! — Evan Teran, Jan 24 '13 at 19:13
@Emil: if your code is in Qt, then it is c++, NOT c. They are different languages. And in c++, there are MUCH easier ways to do this. — Evan Teran, Jan 24 '13 at 19:24
No Qt to be seen. Nor C++. Note that using proper C++ (and/or Qt) types, your issue would be gone. — Frank Osterfeld, Jan 24 '13 at 19:27
@EvanTeran I concur. With the updated code the line-count algorithm before the actual code-loop leaks like a sieve (i.e. the first while-loop, if `getline()` does, in fact allocate memory). — WhozCraig, Jan 24 '13 at 19:27
@Emil: BTW, i just tried your code **verbatum** and there is no crash here. — Evan Teran, Jan 24 '13 at 19:35
@Evan Teran Maybe you are right.When I run the code from the console in linux it works fine but when I run it from linux it crashes — Emil Grigore, Jan 24 '13 at 19:37
@Emil: have you tried running in a debugger to see where the actual crash is? — Evan Teran, Jan 24 '13 at 19:38
@Emil Grigore : When I try strcpy the program crashes.Is a malloc problem? I am very sure that i is big enough. Yes!you need to test for NULL — qPCR4vir, Jan 25 '13 at 09:58

score 3 · Accepted Answer · answered Jan 24 '13 at 19:19

3

There are a couple of issues with the code, i'll comment with what I believe should work...:

 // I **assume** that these are the definitions for these variables 
 // based on your comments
 size_t len = 0;
 char *line = NULL;
 ssize_t read1;

 // I **assume** that i has a reasonable value here, but this is not good to assume, 
 // what if the file is a line longer tomorrow? I hope that you calculate the number 
 // of lines somehow, that would be "less bad"
 int i = 10; // 10 lines in the file, who knows ?!?
 char **arr;

 // don't bother casting...
 arr = malloc(i * sizeof(char*)); 
 i=0;

 while((read1 = getline(&line, &len, fp)) != -1) {

     // THIS LINE DOES NOTHING, so we can just remove it
     // line[strlen(line)]='\0';

     arr[i] = line; // since you asked getline to allocate a buffer for 
                    // you (line was NULL), you can just store the buffer directly
                    // it's YOURS
     i++;

     // THIS IS THE BIG ONE:
     // it is needed because otherwise the NEXT call to getline will 
     // reuse the same buffer, which may not be big enough
     line = NULL;
 }

Also, later for cleanup you should do something like this:

int j;
for(j = 0; j < i; ++j) {
    free(arr[j]);
}
free(arr);
arr = NULL; // not necessary, but good practice to avoid double frees and such

answered Jan 24 '13 at 19:19

Evan Teran

87,561
32
179
238

Thank you but is still doesn't work.I will edit the code to be more clear. – Emil Grigore Jan 24 '13 at 19:23
+1: @EmilGrigore please do, because if `getline()` does what this prescribes it does, this code is correct (save for the missing check of `i`, which is easily the worst named variable in this code block. – WhozCraig Jan 24 '13 at 19:24
@Emil, please define "doesn't work" because the code I supplied should not crash to the best of my knowlege. have you tried adding the "re`NULL`ing" of line like in my code? – Evan Teran Jan 24 '13 at 19:27
@Evan Teran Yes i tried but it still crashes. – Emil Grigore Jan 24 '13 at 19:30
@Emil:: also, based on your edits, you need to set `line` to `NULL` before the second loop (the one which actually reads the lines). Because the earlier counting loop set `line` to a non-`NULL` value. – Evan Teran Jan 24 '13 at 19:31
I tried this but I still get a crash – Emil Grigore Jan 24 '13 at 19:35
@Evan Teran I managed to make it work.It seemed that I had to toggle some qt settings....I never used qt before.Thank you very much and sorry for the trouble – Emil Grigore Jan 24 '13 at 19:49
line = NULL; len=0; ?? – qPCR4vir Jan 24 '13 at 19:52
@Evan Teran: the reason could be simple he is not testing for NULL after malloc?? – qPCR4vir Jan 25 '13 at 10:03

score 2 · Answer 2 · edited May 23 '17 at 12:21

2

You dont test if you have actualy more lines than the original i

 arr=(char**)malloc(i_ori*sizeof(char*));//i_ori is a variable i use to know the number of lines
 i=0;

 while((read1=getline(&line,&len,fp))!=-1 && i<i_ori)

Also, you never test if malloc return NULL !! See https://stackoverflow.com/a/2280342/1458030

@Emil Grigore : When I try strcpy the program crashes.Is a malloc problem? I am very sure that i is big enough.

Yes! you need to test for NULL.

If you are using C++ and Qt, why not containers, streams?

edited May 23 '17 at 12:21

Community

1
1

answered Jan 24 '13 at 19:10

qPCR4vir

3,521
1
22
32

I am sure I have the same number of lines as i – Emil Grigore Jan 24 '13 at 19:13
@EmilGrigore: Then please edit your post to include a [minimal test-case](http://sscce.org) that demonstrates this. – Oliver Charlesworth Jan 24 '13 at 19:18

strcpy char pointer abort

2 Answers2