Program keeps returning Segmentation Fault

Question

I'm a new person who loves to play around with coding. Recently I was going through a course on edx, and one of the exercises I need to complete has this small code snippet that keeps on giving Segmentation fault. I have taken out the faulty bit (everything else compiles nicely)

#include <stdio.h>
#include <string.h>
#include <cs50.h>
#include <ctype.h>
#include <stdlib.h>

int main (int argc, string argv[])
{
    if (argc == 2 && isalpha(argv[1]))
    {
        int a = 0;

        while (argv[1][a] == '\0')
        {
            a++;
            printf("%c\n", argv[1][a]);
        }
    }
    else
    {
        printf("Usage: ./programname 1-alphabetical word\n");
        return 1;
    }
}

The problem seems to be here: argv[1][a] but I can't for the life of me find out what, and how to fix it.

you presumably meant `while (argv[1][a] != '\0')` and to increment `a` _after_ trying to print from it. What you have here will index out of range and/or loop forever. — underscore_d, Jul 28 '17 at 12:11
`isalpha(argv[1])` --> `isalpha(argv[1][0])` or `isalpha(*argv[1])` — BLUEPIXY, Jul 28 '17 at 12:11
Also, (A) just use a debugger, as it trivialises basic logic fails like this, and (B, although I can't quite tell whether this is what you meant you did,) don't remove code that you don't think is relevant — underscore_d, Jul 28 '17 at 12:14
where do you get the segmentation fault? run your code in a debugger and find out so you can finish your question — Chris Turner, Jul 28 '17 at 12:15
What type is `string`? This doesnt look like plain C to me, or it is missing important types — dhein, Jul 28 '17 at 12:19
kudos for narrowing your program to just the part that causes the error. It's a great skill that eludes many novices. — bolov, Jul 28 '17 at 12:19
@underscore_d: I disagree. Constructing a [MCVE] that reproduces the problem without distractions is 100% the right thing to do — Asteroids With Wings, Jul 28 '17 at 12:22
@AsteroidsWithWings Of course, I don't disagree with that. My comment stemmed I think from a misreading of what they said here: _"I have taken out the faulty bit"_ and thought they meant they had left out some other bad code, but they must mean they have _posted_ only the faulty bit. — underscore_d, Jul 28 '17 at 12:23
@underscore_d yeah they took out the faulty bit and posted it here — Asteroids With Wings, Jul 28 '17 at 12:23
Your usage message (`printf("Usage: ./programname 1-alphabetical word\n");`) is OK, but it would be better as `fprintf(stderr, "Usage: %s alphabeticalword\n", argv[0]);` where the message is written to standard error and includes the value from `argv[0]`. The message as given seems to imply that you need two arguments, but the test indicates one, so I revised the message there, too — I'd probably just use `word` rather than `alphabeticalword`, but that's closer to what you had before. (This is a minor refinement. Many programmers don't do anything like as good a job as you've done.) — Jonathan Leffler, Jul 30 '17 at 20:26

Theo Walton · Answer 1 · 2017-07-28T12:29:57.337

0

isalpha(argv[1]) looks incorrect and should probably be isalpha(argv[1][0])

isalpha takes a character but you entered in a string to the function

another thing that sticks out as wrong is argv[1][a] == '\0' the == should be != this will mean that the while loop will stop once it hits the \0

perhaps

if (argc == 2)
{
    int a = 0;

    while (argv[1][a] != '\0')
    {
                if (isalpha(argv[1][a])
                    printf("%c\n", argv[1][a]);
                a++;
    }
}

may be what you are looking for?

edited Jul 28 '17 at 12:29

answered Jul 28 '17 at 12:14

Theo Walton

1,085
9
24

This question didn't really need to be asked, but an answer should at least be complete and verified. While correct, that's not the only problem with the code given, and it doesn't sound like you tested whether it fixed anything/everything. – underscore_d Jul 28 '17 at 12:15
ok sorry ill try to edit to include more info – Theo Walton Jul 28 '17 at 12:16
not sure how only checking one letter helps - OP probably expected `isalpha` to check the whole string – Asteroids With Wings Jul 28 '17 at 12:22

underscore_d · Accepted Answer · 2017-07-28T14:18:18.917

(1) isalpha(argv[1]) is wrong. This function expects a single character, but you are passing a pointer-to-string. That will certainly not give you any kind of expected result, and it's probably Undefined Behaviour into the bargain. You either need to loop and check each character, use a more high-level library function to check the entire string, or - as a quick and probably sense-changing fix - just check the 1st character as BLUEPIXY suggested: isalpha( argv[1][0] ) or isalpha( *argv[0] ).

(2) Your while condition is wrong. You are telling it to loop while the current character is NUL. This will do nothing for non-empty strings and hit the next problem #3 for an empty one. You presumably meant while (argv[1][a] != '\0'), i.e. to loop only until a NUL byte is reached.

(3) You increment index a before trying to printf() it. This will index out of range right now, if the input string is empty, as the body executes and then you immediately index beyond the terminating NUL. Even if the loop condition was fixed, you would miss the 1st character and then print the terminating NUL, neither of which make sense. You should only increment a once you have verified it is in-range and done what you need to do with it. So, printf() it, then increment it.

2 and 3 seem most easily soluble by using a for loop instead of manually splitting up the initialisation, testing, and incrementing of the loop variable. You should also use the correct type for indexing: in case you wanted to print a string with millions or billions of characters, an int is not wide enough, and it's just not good style. So:

#include <stddef.h> /* size_t */

for (size_t a = 0; argv[1][a] != '\0'; ++a) {
    printf("%c\n", argv[1][a]);
}

Thank you. This worked perfectly. In hindsight it was such a stupid mistake. But it was satisfying to complete the problem at least.Thank you! — Muhammad Sohaib, Jul 28 '17 at 13:06

Vlad from Moscow · Answer 3 · 2017-07-28T12:46:05.853

The only reason for the segmentation fault I see is this subexpression of the if statement

if (argc == 2 && isalpha(argv[1]))
                 ^^^^^^^^^^^^^^^^

There is specified an argument of an incorrect type. The expression argv[1] has the type char * while the function requires an object of character type that is interpreted as unsigned char and promoted to the type int.

So when the promoted argument has a negative value (except the EOF value) the function isalpha has undefined behavior.

From the C Standard (7.4 Character handling <ctype.h>)

1 The header <ctype.h> declares several functions useful for classifying and mapping characters.198) In all cases the argument is an int, the value of which shall be representable as an unsigned char or shall equal the value of the macro EOF. If the argument has any other value, the behavior is undefined.

You should write the if statement either like

if (argc == 2 && isalpha( ( unsigned char )argv[1][0] ) )

or like

if (argc == 2 && isalpha( ( unsigned char )*argv[1] ) )

Also there is a bug in the while statement that will execute never if the argument is not an empty string. I think you mean the following

    int a = 0;

    while ( argv[1][a] != '\0' )
    {
        printf("%c\n", argv[1][a]);
        a++;
    }

or for example like

    int a = 0;

    while ( argv[1][a] )
    {
        printf("%c\n", argv[1][a++]);
    }

Program keeps returning Segmentation Fault

3 Answers3