segmentation fault when trying using strlen on argv[1]

Question

I have been trying to do cs50's course (this course was my first introduction to c ever, please have mercy) and got to the substitution problem. I have been stuck on it for a while now. this is my buggy code:

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

int check(string str);

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


    if(argc < 1 || argc > 2)
    {
        printf("Usage: ./substitution key");
        return 1;
    }

    string key = argv[1];

    if(check(key) == 1)
    {
            return 1;
    }



    string plaintext = get_string("plaintext:");
    int lenght = strlen(plaintext);
    char ciphertext[lenght + 1];

    for(int i = 0; i <= lenght; i++)
    {
        if(islower(plaintext[i]))
        {
            plaintext[i] -= 97;
            ciphertext[i] = key[(int)plaintext[i]];
            if(isupper(ciphertext[i]))
            {
                ciphertext[i] = tolower(ciphertext[i]);
            }
        }

        if(isupper(plaintext[i]))
        {
            plaintext[i] -= 65;
            ciphertext[i] = key[(int)plaintext[i]];
            if(islower(ciphertext[i]))
            {
                ciphertext[i] = toupper(ciphertext[i]);
            }
        }
    }

    printf("ciphertext:%s", ciphertext);





}




int check(string str)
{
    //controlle del numero di lettere, che i caratteri della chiave non siano ripetuti o numerici hahahahah lol
   char alphabet[26] = {'a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z'};
   int count[26] = { 0 };
   if(strlen(str) != 26)
   {
    printf("Key must contain 26 characters.");
    return 1;
   }
   for(int i = 0; i < 25; i++)
   {
        if(isalpha(str[i]) == 0)
        {
            return 1;
        }

        str[i] = (char)tolower(str[i]);

        for(int j = 0; j < 25; j++)
        {
            if(str[i] == alphabet[j])
            {
                count[j] += 1;
            }

        }
    }

    for(int i = 0; i < 25; )
    {
        if(count[i] != 1)
        {
            return 1;
        }
    }

    return 0;

}

the main issue is that it seems impossible to handle argv[1] as a string by storing it, because whenever I run the program it just keeps running endlessly.

This is the segmentation error i get on the debug console: Program received signal SIGSEGV, Segmentation fault. __strlen_evex () at ../sysdeps/x86_64/multiarch/strlen-evex.S:77

When I try to debug it says a segmentation fault has occured when trying to get the key lenght with strlen(). also when debugging and it says that key has stored 0x0, so maybe I somehow got wrong the assigning of argv[1] to key as well. I think it could be that argv[1] doesn't end with a

NULL character but I'm not sure as I only know the basics of c.

If `argc==1`, then `argv[1]` contains `NULL`. Don't put that into `strlen`! If you expect an argument to your program, then `argc` must be at least 2. — Gerhardh, Jun 28 '23 at 10:16
And in the `check` function, how many characters will the loop `for(int i = 0; i < 25; i++)` really check? Remember that the value `25` will *not* be used as an index. — Some programmer dude, Jun 28 '23 at 10:21
cs50 `string` is a deceptive type which is actually a pointer to `char`. Assignment doesn't copy strings, it only copies addresses. — Swift - Friday Pie, Jun 28 '23 at 10:37
@someprogrammerdude yes, you are right. I immediately fixed it. thanks! — gondola, Jun 28 '23 at 11:18

gsamaras · Accepted Answer · 2023-06-29T13:27:46.727

Here is a complete minimal example summarizing the changes discussed:

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

int check(string str);

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

    if(argc != 2)
    {
        printf("Usage: ./substitution key");
        return 1;
    }

    string key = argv[1];

    if(check(key) == 1)
    {
            return 1;
    }

    string plaintext = get_string("plaintext:");
    int lenght = strlen(plaintext);
    char ciphertext[lenght + 1];
    memset(ciphertext, 0, sizeof(ciphertext));

    for(int i = 0; i < lenght; i++)
    {
        if(islower(plaintext[i]))
        {
            plaintext[i] -= 97;
            ciphertext[i] = key[(int)plaintext[i]];
            if(isupper(ciphertext[i]))
            {
                ciphertext[i] = tolower(ciphertext[i]);
            }
        }

        if(isupper(plaintext[i]))
        {
            plaintext[i] -= 65;
            ciphertext[i] = key[(int)plaintext[i]];
            if(islower(ciphertext[i]))
            {
                ciphertext[i] = toupper(ciphertext[i]);
            }
        }
    }

    printf("ciphertext:%s\n", ciphertext);
}

int check(string str)
{
    //controlle del numero di lettere, che i caratteri della chiave non siano ripetuti o numerici hahahahah lol
   char alphabet[26] = {'a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z'};
   int count[26] = { 0 };
   if(strlen(str) != 26)
   {
    printf("Key must contain 26 characters.");
    return 1;
   }
   for(int i = 0; str[i] != 0; i++)
   {
        if(isalpha(str[i]) == 0)
        {
            return 1;
        }

        str[i] = tolower(str[i]);

        for(int j = 0; j < 26; j++)
        {
            if(str[i] == alphabet[j])
            {
                count[j] += 1;
            }

        }
    }

    for(int i = 0; i < 26; i++)
    {
        if(count[i] != 1)
        {
            return 1;
        }
    }

    return 0;

}

Output:

./substitution ABCDEFGHIJKLMNOPQRSTUVWXYZ
plaintext:example
ciphertext:example
$

Below a step-by-step explanation of the changes:

Try changing:

if(argc < 1 || argc > 2)

to:

if(argc != 2)

like @Someprogrammerdude commented. Remember that argc is the number of arguments on the command line (including the program name itself).

Moreover, in check() you are assuming that str is going to be of length 25, judging from this:

for(int i = 0; i < 25; i++)
{
     if(isalpha(str[i]) == 0)
     ...

Remember that C-strings are NULL terminated, so you should do instead:

for(int i = 0; str[i] != 0; i++)
{
     if(isalpha(str[i]) == 0)
     ...

in order to not go out of bounds of str.

Furthermore, change this:

    for(int j = 0; j < 25; j++)
    {
        if(str[i] == alphabet[j])

to:

    for(int j = 0; j < 26; j++)
    {
        if(str[i] == alphabet[j])

since you want to iterate from 0 to 25, not to 24.

Now here:

for(int i = 0; i < 25; )
{
    if(count[i] != 1)
    {
        return 1;
    }
}

you are going into an infinite loop, unless count[0] is different than 1. You need to increase the i counter defined in the scope of this for loop. You also need to change 25 to 26, so that you iterate over the entire counts array (including the last element, count[25]); so change it to this:

for(int i = 0; i < 26; i++)
{
    if(count[i] != 1)
    {
        return 1;
    }
}

You forgot to NULL-terminate the cipher text string, that's why you go out of bounds when using string methods from the C standard library (which requires all strings to be NULL terminated). So change this:

char ciphertext[lenght + 1];

to:

char ciphertext[lenght + 1];
memset(ciphertext, 0, sizeof(ciphertext));

as explained in C compile error: "Variable-sized object may not be initialized".

Then you want to iterate over the plain text and the cipher text, where, so you need to check that the counter of the for loop does not exceed the length of the string with the minimum length, in this case, the plaintext, so consider changing this:

for(int i = 0; i <= lenght; i++)

to:

for(int i = 0; i < lenght; i++)

With this change the last character of the ciphertext will be always NULL however, so you may want to rethink your logic there, depending on what you want to achieve.

@Jens I meant to change it, but frankly speaking, I got caught up in the Hot Network Questions on the right, sorry! Fixed, thanks anyway for pointing out regardless and for the downvote in order to alert me! :) — gsamaras, Jun 28 '23 at 10:35
@gsamaras thanks for the insight, it was really helpful! But I still have problems with the strlen() function, I think it keeps going "out of bounds" as you said. The only thing that I think could be the problem is that the funcion keeps on counting for some reason. — gondola, Jun 28 '23 at 11:22
@gsamaras, Aside: `memset(ciphertext, 0, (lenght + 1) * sizeof(char));` simpler with `memset(ciphertext, 0, sizeof ciphertext);`. — chux - Reinstate Monica, Jun 28 '23 at 14:38
"C compile error: "Variable-sized object may not be initialized", IIRC, C23 will allow it. — chux - Reinstate Monica, Jun 28 '23 at 14:38
"Minor: No need to cast the return value of tolower()" --> It is a narrowing (`int` to `char`) and some warnings complain about it, unless a cast is used. — chux - Reinstate Monica, Jun 28 '23 at 14:41
@chux-ReinstateMonica ok for the `memset()`. For C23 I couldn't check, because I was using an online complier to use the cs50 library and it doesn't support C23. Are you talking about https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2778.pdf? Good catch for `tolower()`. — gsamaras, Jun 29 '23 at 10:00
@gsamaras See [Consistent, Warningless, and Intuitive Initialization with {}](https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2900.htm). `memset(ciphertext, 0, sizeof ciphertext)` also useful in "You forgot to NULL-terminate ..." part. — chux - Reinstate Monica, Jun 29 '23 at 11:41
Ah @Jens sorry, but no worries even you had! :) Chux, I edited only in the full example, not the step-by-step explanation, sorry, re-edited, thanks! — gsamaras, Jun 29 '23 at 13:29

score 0 · Answer 2 · answered Jun 28 '23 at 14:44

0

I found the problem... it was an unfinished for loop.

for(int i = 0; i < 25; )<---
{
    if(count[i] != 1)
    {
        return 1;
    }
}
return 0;

answered Jun 28 '23 at 14:44

gondola

13
2

segmentation fault when trying using strlen on argv[1]

2 Answers2