Segmentation fault in C program with apparently perfect code

Question

I recently recommended K&R to a friend who wanted to learn C. He came across an exercise in the first chapter that gave him various errors. I compiled it on my Ubuntu installation, alternating between the C90 option and the defaults. I've looked at every angle but it seems to be perfect code...yet it consistently gives me a segmentation fault each time I run it. I'm not the sharpest programmer in the shed but this has me pretty frustrated.

What on earth is causing such an error?

Here is the code:

#include <stdio.h>
#define MAXLINE 1000

void reverse(char s[]);

/* A program that reverses its input a line at a time */
main()
{
    int c, i;
    char line[MAXLINE];

    for (i = 0; (c = getchar()) != EOF; ++i) {
        line[i] = c;

        if (c == '\n') {        /* Upon encountering a newline */
            line[i] = '\0';     /* replace newline with null terminator */
            i = 0;
            reverse(line);
            printf("\n%s\n", line);
        }
    }
    return 0;
}

/* A function that reverses the character string */
void reverse(char s[])
{   
    int a, z;
    char x;

    for (z = 0; s[z]; ++z)      /* Figure out where null terminator is */
        ;
    --z;
    for (a = 0; a != z; ++a) {  /* Reverse array usinng x as placeholder */
        x = s[a];
        s[a] = s[z];
        s[z] = x;
        --z;
    }
}

Not sure how you're getting a segfault, it is an infinite loop though. — Kevin, Nov 05 '13 at 04:19
Yeah I posted an earlier version of the code, it's missing a null statement in the for. But if you add the semicolon it gives a seg fault. — , Nov 05 '13 at 14:01

DaoWen · Accepted Answer · 2013-11-05T04:41:10.423

You're missing a semi-colon here:

for (z = 0; s[z]; ++z);     /* Figure out where null terminator is */
                   // ^

This loop is supposed to run until it finds the null terminator. If you leave off this semi-colon, then on each iteration it does both ++z and --z, which means it just loops forever. You want the --z to happen after this loop has completed, as that will set z equal to the last character before the string's null terminator.

For an even-length string, a and z will cross each other (they'll never be equal) in the second loop. For example, if z=5 and a=4, then on the next iteration a=5 and z=4. If you check for a<z instead of a!=z then you avoid that problem. Since you're checking for != instead of <, this would cause the loop to run pretty much infinitely. However, you'll end up with a SEGFAULT as a grows too large and z grows too small, since they'll both be used to index into memory outside your buffer.

for (a = 0; a != z; ++a) {  /* Reverse array usinng x as placeholder */
          //  ^ should be <, not !=

Finally, there's also a bug in main. When you find a newline, you set i=0. This works fine when you print the string, but when i is incremented at the end of the loop, you end up with i=1 as you start reading the next string. This means you'll have an extra character at the beginning of your string. You need to do something to properly reset i to make sure that doesn't happen.

K&R has a string reverse function in Section 3.5. In my copy it's on page 62. It looks like your friend decided to iterate over the string rather than calling strlen (which is really all the function call does anyway), and thought that a<z should be equivalent to a!=z.

The first item is the reason I never use `for (...);`. Instead, I use `for (...) continue;` which is functionally the same but harder to mis-read/mess-up. — DoxyLover, Nov 05 '13 at 06:01
The infinite loop was a posting mistake, but yeah wow it runs beautifully now. Never thought I'd see the day where I'd be happy over reading words backwards in the console lol. Nice work catching the i reset too. — , Nov 05 '13 at 14:26

score 0 · Answer 2 · answered Nov 05 '13 at 04:11

0

Does this loop over run?

for (z = 0; s[z]; ++z)      /* Figure out where null terminator is */
        --z;

answered Nov 05 '13 at 04:11

Pat Mustard

1,852
9
31
58

No, it's just an infinite loop. – Kevin Nov 05 '13 at 04:18
Yes, but the infinite loop references a finite array so at some point it will over run the array boundary or give an int overflow: http://stackoverflow.com/questions/15177018/c-array-allocation-segmentation-fault-11-newbie – Pat Mustard Nov 05 '13 at 04:47
It's a finite array, yes, but only `s[0]` is ever accessed. There is a `z++` and a `z--` so the index never changes. – Kevin Nov 05 '13 at 05:09
It's infinite but its my error, the seg fault turned out to be caused by the test in the for loop directly after. – Nov 05 '13 at 14:18

score 0 · Answer 3 · answered Nov 05 '13 at 04:14

0

for (z = 0; s[z]; ++z)      /* Figure out where null terminator is */
        --z;

Just after the first iteration, z becomes -1 -- so trying to access s[z] results in segmentation fault.

answered Nov 05 '13 at 04:14

Siddhartha Ghosh

2,988
5
18
25

1

Except that the `++z` is executed before the `s[z]` check. It's always `s[0]`. – Kevin Nov 05 '13 at 04:17

Segmentation fault in C program with apparently perfect code

3 Answers3