Segmentation fault in a for loop

Question

I'm getting a segmentation fault for this code and I can't, for the life of me, figure out why. I'm rather new to pointers so it could be something obvious. The code is supposed to take two lines of integers from stdin and print them out in an alternating fashion. I haven't finished the last part of the code which prints out the alternating numbers.

    int main(){
        char *str1 = NULL;
        char *str2 = NULL;
        size_t sz = 0;
        int i;
        int x;
        char *rp1 = NULL;
        char *rp2 = NULL;
        getline(&str1, &sz, stdin);
        getline(&str2, &sz, stdin);
        char *result1;
        result1 = malloc(sizeof(*str1));
        char *result2; 
        result2 = malloc(sizeof(*str2));
        for(i = 0; (x = sscanf(str1, "%s ", &result1[i])) > 0; i++){
            if(x == EOF){
               return 0;
            }
            if(!isdigit(result1[i])){
               fprintf(stderr, "Error: invalid non-integer input\n");
               return 1;
            }
        }
        rp1 = malloc(i);
        rp1 = result1;
        for(i = 0; sscanf(str2, "%s ", &result2[i]) > 0; i++){
            if(!isdigit(result2[i])){
            fprintf(stderr, "Error: invalid non-integer input\n");
                return 1;
            }
        }
        rp2 = malloc(i);
        rp2 = result2;
        return 0;
  }

When I run gdb, it says I'm getting the segmentation fault at line 25, which is the first for loop.

Edit: So I've fixed one issue which was in the malloc function before the for loops, but I'm still getting the segmentation fault.

When you ran it under your debugger, which line generated the segfault? — Martin James, Mar 04 '16 at 21:50
Yeah woops sorry I was adding that right when you replied. gdb says line 25, which is the first for loop. — moo, Mar 04 '16 at 21:52
if str1 is a pointer, why are you trying to malloc the size of a pointer? Shouldnt you malloc the size of what it is pointing to? — Fallenreaper, Mar 04 '16 at 21:53
'result1 = malloc(sizeof(str1));' does not do what you seem to think it does:( — Martin James, Mar 04 '16 at 21:54
Ah yes i've been changing that line over and over. Should it be the dereferenced pointer in the malloc function or does that not work either? — moo, Mar 04 '16 at 21:56
`sizeof(*str1)` is most likely 1 because `*str1` is a `char`, which is typically one byte. What you actually want is `strlen(str1)+1` (add 1 to account for the null terminator) — Olipro, Mar 04 '16 at 22:18
`result1` and `result2` should be `char**` if you want to store multiple strings in them, and you will need to allocate each element of them before you can use it. — , Mar 04 '16 at 22:19
I don't think so. I'm not concatenation per say, I'm reading individual values from each line and outputting them one by one and alternating the lines. So an input of "12 13 14" and "1 2 3" would ouput "12 1 13 2 14 3" — moo, Mar 04 '16 at 22:19
"Another measure of the function is the number of local variables. They shouldn't exceed 5-10, or you're doing something wrong. Re-think the function, and split it into smaller pieces. A human brain can generally easily keep track of about 7 different things, anything more and it gets confused." -https://www.kernel.org/doc/Documentation/CodingStyle — Dmytro, Mar 04 '16 at 22:24
So you know, `atoi()` already exists to convert strings into integers. I think that's what you're trying to accomplish? — Schwern, Mar 04 '16 at 22:31

score 1 · Accepted Answer · answered Mar 04 '16 at 22:24

1

Problem 1:

sizeof(*str[1-2]) is the same as sizeof(char). You are allocating a single char for both result buffers, which is clearly insufficient. Use strlen(str[1-2]) + 1 this will give you a buffer equal to the number of chars in the string, plus one more for the null terminator.

Problem 2:

getline() allocates a buffer which you are expected to free() after you're done with it, you are not doing so which is effectively a memory leak. This is not the cause of your problem, but it bears mentioning. The same is true of all your malloc()'d memory.

answered Mar 04 '16 at 22:24

Olipro

3,489
19
25

Also, 'If *lineptr is set to NULL AND *n is set 0 before the call, then getline() will allocate a buffer for storing the line.' Both calls to getline() reference 'sz' which, for the second call, will not be zero:( – Martin James Mar 04 '16 at 22:38
Because the first argument is NULL, `sz` is ignored anyway, he could have just passed NULL instead of `&sz` – Olipro Mar 04 '16 at 22:51

score 1 · Answer 2 · answered Mar 04 '16 at 22:35

There's a lot of issues with this code. Other people have pointed out the malloc problems, but on the whole it's unnecessarily complicated. All the intermediate variables can be eliminated and str1 and str2 accessed directly.

To simplify things, I'm going to reduce this to just one input string, str. If you want two, the code should be put into a function rather than copied.

The for + sscanf loops appear to be trying to iterate through str and checking that it contains only digits. This is much better accomplished by walking through the character array until you hit the null character on the end.

for(int i = 0; str[i] != '\0'; i++){
    ...do something with str[i]...
}

So checking that each character of input is a digit...

for(int i = 0; str[i] != '\0'; i++){
    if(!isdigit(str[i])){
        fprintf(stderr, "Error: invalid non-integer input '%c'\n", str[i]);
        return 1;
    }
}

This will fail because str, being read from stdin, has a newline on the end.

Thank you! I'm going to make a new attempt at it seeing as though I have so many complicated issues. — moo, Mar 05 '16 at 00:21

Segmentation fault in a for loop

2 Answers2