Can anyone tell me why im getting a Segmentation Fault (cs50 substitution)

Question

In my program I get the errors that I controlled, but when I try to avoid those preset errors and enter what I want the user to enter, it returns a segmentation fault.

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

int main(int argc, string argv[])
{
    string key = argv[1];
    if (argc < 2 || argc > 2)
    {
        printf("Usage: ./substitution key\n");
        exit(1);
    }
    
    if (argc == 2 && strlen(key) < 26)
    {
        printf("Key must contain 26 characters.\n");
        exit(1);
    }
    
    if (strlen(key) > 26)
    {
        printf("Key must contain 26 characters.\n");
        exit(1);
    }
    
    for (int i = 0; i < 26; i++)
    {
        if (isalpha(key) == 0)
        {
            printf("MAKE SURE YOUR KEY ONLY HAS LETTERS\n");
        }
    }
    printf("test");
}

You should check for the right number of command line parameters before using them. This is also wrong: `if (isalpha(key) == 0)`should be `if (isalpha(key[i]) == 0)`. You don't need to check for `argc == 2` after you've already decided it is, and if you could calculate the length once and check if it is `!=` 26 to have less duplicate code. Similarly `if (argc != 2)` seems more clear to me. — Retired Ninja, Jul 25 '20 at 03:45
BTW, gdb is a great friend. `g++ -g .c` then use `gdb --args ./a.out `. Now type `r`. If crashes, `bt` for backtrace. And of course learn other things like break point `b ` and step through `n` or step in `s` which will make your life very easy — Bhaskar Tallamraju, Jul 25 '20 at 04:22

score 0 · Accepted Answer · edited Jul 25 '20 at 14:38

0

@Retired Ninja is spot on. isalpha on key was causing the segmentation. Here is a slightly optimized way of the code.

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

int main(int argc, string argv[])
{
    if (argc != 2)
    {
        printf("Usage: ./substitution key\n");
        exit(1);
    }
    
    string key = argv[1];
    int keylen = strlen(key);
    
    if (keylen != 26)
    {
        printf("Key must contain 26 characters.\n");
        exit(1);
    } 
    
    for (int i = 0; i < 26; i++)
    {
        unsigned char const c = key[i];
        if (!isalpha(c))
            printf("MAKE SURE YOUR KEY ONLY HAS LETTERS\n");
    }
    printf("test\n");
}

edited Jul 25 '20 at 14:38

Kuba hasn't forgotten Monica

95,931
16
151
313

answered Jul 25 '20 at 04:17

Bhaskar Tallamraju

314
1
7

The argument of `isalpha` should be cast to `unsigned char`: `isalpha((unsigned char)key[i])`. Casting it to `int` is redundant and simply wrong. – M. Nejat Aydin Jul 25 '20 at 05:41
it is not. Please do a man on isalpha. Passing the value is what is required. Here is the prototype from manual pages: `int isalpha(int c);` – Bhaskar Tallamraju Jul 25 '20 at 07:01
2

*It is*. Please read [Arguments to character-handling functions must be representable as an unsigned char](https://wiki.sei.cmu.edu/confluence/display/c/STR37-C.+Arguments+to+character-handling+functions+must+be+representable+as+an+unsigned+char) and [Cast characters to unsigned char before converting to larger integer sizes](https://wiki.sei.cmu.edu/confluence/display/c/STR34-C.+Cast+characters+to+unsigned+char+before+converting+to+larger+integer+sizes). – M. Nejat Aydin Jul 25 '20 at 07:38
2

Also read [ctype.h](http://port70.net/~nsz/c/c11/n1570.html#7.4): 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 (emphasize mine). – M. Nejat Aydin Jul 25 '20 at 07:40
2

Also the manual page of `isalpha` doesn't support you. [isalpha](https://man7.org/linux/man-pages/man3/isalpha.3.html): These functions check whether c, **which must have the value of an unsigned char or EOF** ... (emphasize mine) – M. Nejat Aydin Jul 25 '20 at 07:47
2

This is discussed a few times here: [Does ctype.h still require unsigned char](https://stackoverflow.com/questions/17975913/does-ctype-h-still-require-unsigned-char) and [Do I need to cast to unsigned char before calling toupper(), tolower(), et al.?](https://stackoverflow.com/questions/21805674/do-i-need-to-cast-to-unsigned-char-before-calling-toupper-tolower-et-al). The verdict is that it is necessary to cast to `unsigned char` unless it is known that the value of the character in the argument cannot be negative. And, of course, casting the argument to `int` is completely unnecessary. – Lxer Lx Jul 25 '20 at 09:11
I agree, I was merely stating the parameter required for isalpha. Moreover, the value needs to be of value unsigned char as stated in manual page but typecasting to unsigned char is not needed as well in this case @ M. Nejat Aydin. I agree with @Lxer Lx that typecasting to int is unnecessary as well. – Bhaskar Tallamraju Jul 25 '20 at 13:44
1

The cast to `unsigned char` is necessary to prevent the parameter (`int`) from being *sign-extended* should it be within the extended ASCII set where the *sign-bit* would be `1` if passed as a signed-char-- which would be very confusing to the macro. There is also no guarantee that `char` will be a signed value on all implementations -- adding to the fun. – David C. Rankin Jul 25 '20 at 14:18
yes, that is right. This was a good discussion, thanks guys! – Bhaskar Tallamraju Jul 25 '20 at 14:34

Can anyone tell me why im getting a Segmentation Fault (cs50 substitution)

1 Answers1