Why is this my code producing a segmentation fault when there is only argc[1]?

Question

I'm taking a intro CS course (CS50x) and came across a slight problem.

Why is this C code producing a segmentation fault when there is only argc[1]?

It should print the statement. Why won't it print the error?

Thanks in advance!

int main(int argc, string argv[])
{
    //HOUSEKEEPING
    //Get/Validate Key
    string key = argv[1];
    int finalKey = atoi(key) % 26;

    while (argc != 2)
    {
        for (int i = 0; i < strlen(key); i++)

            if (!isdigit(key[i]))
            {
                printf("Usage: ./caesar key\n");
                return 1;
            }

            else
            {
                printf("Usage: ./caesar key\n");
                return 1;
            }
            //...

You need to check if `argc == 2`, before accessing `argv[1]`. — Jabberwocky, Apr 15 '21 at 07:56
Furthermore `while (argc != 2)` is nonsense, as `argc` doesn't change. You don't need a loop here — Jabberwocky, Apr 15 '21 at 07:57
You do not pass the caesar-key. For a caesar-key of 1 (a --> b, b-->c, ...) you would run your program via `./caesar 1`. — gkhaos, Apr 15 '21 at 08:17
What is `string` type? Is it `char *` and if yes - why you do not use `char *`? — i486, Apr 15 '21 at 08:40
@i486 it's CS50 BS. They use `typedef char* string` to make the students somewhat believe that there is a `string` type in C. — Jabberwocky, Apr 15 '21 at 09:29
I've tried using argc == 2 and the same thing happens. And I used a if statement as well to no avail. Yea, the CS50 ide has a header file that makes the char * data type into string. — Al Solomon, Apr 15 '21 at 09:48
@AlSolomon you're doing the test too late. Don't access `argv[1]` unless you know that `argc >= 2`. See my first comment. — Jabberwocky, Apr 15 '21 at 10:01
I tried declaring those variables after testing the condition, but now those variables have become local instead of global. I can no longer use them. — Al Solomon, Apr 15 '21 at 11:03
I will try to create a separate function so they will work. Thank you again for your help! — Al Solomon, Apr 15 '21 at 11:05
@Jabberwocky It is not very good idea because there is `string` class in C++. In addition, if students have "fear" from pointers, it is better to learn python or other language. — i486, Apr 15 '21 at 11:44
@i486 yes I know, but you should tell this the people reponsable of the [cs50](https://cs50.harvard.edu/) course — Jabberwocky, Apr 15 '21 at 11:53

anastaciu · Answer 1 · 2021-04-15T09:25:41.237

argc is an integer argument that keeps track of the number of arguments in the command line, there is no argc[1], there might be an argv[1] if you provide a second command line argument, otherwise trying to read from a non existing argv[x] amounts to undefined behavior, and is the likely culprit of the segmentation fault you are experiencing.

In light of that information you'll notice that the while statement makes little sense, argc remains unchanged so the loop will either never execute, or will be infinite if argc is not 2 (and the program doesn't crash).

The if-else is also fishy, it will always print the message and return, if the character is a digit or is not a digit alike.

Notwithstanding the goal of the program, a syntactically correct code should look more like this:

int main(int argc, string argv[])
{

    if (argc == 2)
    {
        string key = argv[1];

        int finalKey = atoi(key) % 26; //*

        for (int i = 0; i < strlen(key); i++)
        {
            if (!isdigit(key[i]))
            {
                printf("Usage: ./caesar key\n");
                return 1;
            }
        }
    }
    else
    {
        puts("Wrong number of arguments, usage: ./caesar key\n");
    }
}

^* Note that atoi is quite unsafe, consider using strtol. And you can operate directly in argv[1] i.e. int finalKey = atoi(argv[1]) % 26;

score 1 · Answer 2 · answered Apr 15 '21 at 08:16

I think you are not clear about the operation of argc and argv. argc indicates the number of parameters passed to the main function. At least it will receive one argument (argv[0], which will be the name of the compiled C code). In addition, it will receive all the parameters that you pass through the command line.

In summary, the correct way to do it would be:

if (argc >=2){
        string key = argv[1];
}

Otherwise, you will have a segmentation fault when you try to access the argv[1] value.

Why is this my code producing a segmentation fault when there is only argc[1]?

2 Answers2