CODEWITHSUNDEEP
csegmentation-fault
user2954900
user2954900

Reputation:

Segmentation fault in C program with apparently perfect code

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;
    }
}

Upvotes: 0

Views: 326

Answers (3)

DaoWen
DaoWen

Reputation: 33029

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.

Upvotes: 3

Siddhartha Ghosh
Siddhartha Ghosh

Reputation: 3198

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.

Upvotes: 0

Pat Mustard
Pat Mustard

Reputation: 1902

Does this loop over run?

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

Upvotes: 0

Related Questions