caesar ciphering (segmentation fault)

Question

I'm trying to write a program that Caesar ciphers the string given by the user, but every time I try running it the error "Segmentation Fault" pops up. What am I doing wrong?

    int main(int argc, string argv[])
{
    if (argc != 2)
    {
        return 1;
    }

    int key = atoi(argv[1]);

    printf("Plaintext: ");
    string Ptext = get_string();
    string cipher = 0;

    if(Ptext != NULL)
    {
        for(int i = 0, n = strlen(Ptext); i < n; i++)
        {
            if(isalpha(i))
            {
                if(isupper(i))
                {
                    cipher += toupper(((i + key) % 26));
                }
                else
                {
                    cipher += tolower(((i + key) % 26));
                }
            }
            cipher += i;
        }
        printf("Ciphertext: %s", cipher);
        printf("\n");
    }
}

Your code looks like C++ to me. `string` doesn't exist in C, nor does string appending. What is `get_string()`? — Felix Guo, Aug 05 '17 at 00:31
Since you are using `string` it appears this is a C++ program, not just C. — Anthony Palumbo, Aug 05 '17 at 00:31
no it is c, I didn't include the libraries here but in my editor I've included a library that contains the get_string() function. — , Aug 05 '17 at 00:35
It's also the case you're testing if `i`, the index value, and not the character at index `i` in your if statements e.g. `Ptext[i]` — Miket25, Aug 05 '17 at 00:36
@FelixGuo This `cs50` course does provide, among other things, a `typedef char *string` ... C programmers hate it for that nonesense ;) — , Aug 05 '17 at 00:39
@AshleyMoe : I've added the tags [tag:cs50] and [tag:Caesar-encryption] since the use of `string` and `get_string()` is aconventional except in the CS50 course. Note also the [CS50](http://cs50.stackexchange.com) site. — Jonathan Leffler, Aug 05 '17 at 00:39
If it is C this program is just one huge Undefined Behaviour. You just do some arithmetics on the pointer, then dereference it so the SegFault is not surprising - I would even say that it is the expected behaviour — 0___________, Aug 05 '17 at 00:40
`SomeStringVariable += ...`. This is wrong. Because we do not know the type of `string` (as it is non-standard), I will assume it is a `typedef char * string`. If you have `char *SomeString` and then you attempt to say, `SomeString +=...`, this will actually attempt to move the string/char pointer, forcing it to refer to a new position in the memory. — Spencer D, Aug 05 '17 at 00:41
You probably do pointer arithmetics when you write `cipher +=`, at least when string is something like `char *` and then, as mentioned before, isUpper(i) should be `isUpper(*(Ptext+i))` resp `isUpper(Ptext[i])` — A Dude, Aug 05 '17 at 00:42
Indeed, as this `string` is just a `typedef` for `char *`, the `+=` will operate on pointer values. Using `char *` instead of this *stupid typedef*, this would have been a little *more* obvious. — , Aug 05 '17 at 00:44
@AshleyMoe `string cipher = 0;` is equivalent to `char *cipher = 0;` which is, as I said, more obious: you're initializing a pointer to 0, so it is the *invalid pointer*. It doesn't point to any usable memory. To achieve what you want to do, you need some memory for your output. The easiest thing to do would be something like `char cipher[1024];` and then index into that to write each single encoded character. — , Aug 05 '17 at 00:47
This `get_string()` seems to return an allocated string from `stdin` input. And OP just assumed `+=` was somehow *overloaded* for `string` to append characters. Well, `string` isn't a type / first-class cititzen in C and cs50 would better not try to make it look like it was.... — , Aug 05 '17 at 00:49
Yes, I deleted it because I meant it to be funny and light but when I re-read it it was mean. And besides I was talking about cipher being incremented from 0, not PText. — hoodaticus, Aug 05 '17 at 00:51

Felix Guo · Answer 1 · 2017-08-05T01:15:00.777

2

Based on what I've read in the comments, lets address some information your post is originally missing. There is a typedef char* string as well as a provided get_string() function. Based on those assumptions, you first want to allocate enough space to store your resulting cipher string.

Strings in C are just allocated character arrays, so you will need to allocate that memory somehow. The best way is to allocate it on the heap using malloc.

Change this line: string cipher = 0; to string cipher = malloc(strlen(Ptext) + 1); This will allocate enough space to store your resulting cipher text. Now, you will need to keep track of the index somehow. Add a int size = 0; Now, change your loop to look like this:

for(int i = 0, n = strlen(Ptext); i < n; i++)
    {
        if(isalpha(i))
        {
            if(isupper(i))
            {
                cipher[size++] = toupper(((i + key) % 26));
            }
            else
            {
                cipher[size++] = tolower(((i + key) % 26));
            }
        }
        else 
        {
            cipher[size++] = i;
        }
    }
    cipher[size] = 0; // for the null terminator

What this will do is the same thing you thought += would do. It will insert the character at the correct location in the allocated character array, then increase the size by 1 so that the next insertion will be in the next location. The final thing to do AFTER printing the string is to call free(cipher); to free up the allocated memory based on malloc

I would recommend looking up how malloc and free works, as well as how character arrays work in C. This is one of the pitfalls of typedef char* string, that is, it abstracts away what a string really is in C, which is a pointer to a char array.

edited Aug 05 '17 at 01:15

answered Aug 05 '17 at 00:55

Felix Guo

2,700
14
20

1

Great answer so far! But I have two remarks: "*// for the null pointer*" is wrong, this is the `NUL` terminator (the end-mark of the string), it's definitely not a *pointer*. And then, it's often easier to use an array instead of `malloc()`. – Aug 05 '17 at 00:59
Oops, you're right, I made a typo. Also, I didn't know if the C version the OP was using would support variable length arrays so I figure it's best to use allocated memory rather than risk another potential error. – Felix Guo Aug 05 '17 at 01:00
1

About VLAs, they are mandatory in C99 and optional in C11, so your concern about this is indeed relevant. But often, using a statically sized buffer is good enough. – Aug 05 '17 at 01:02
1

Agreed. I think if within the course, students haven't learned about dynamic allocation yet, then a static buffer would be fine. In this case they also have access to the exact length required. – Felix Guo Aug 05 '17 at 01:04
1

Don't want to get on your nerves, but if you really want to deal with **any** size, don't use `int`, use `size_t` instead. – Aug 05 '17 at 01:04
1

Whoa. Isn't there an `else` missing?: `cipher[size++] = i;` should only be executed for characters that *aren't* encoded. I see this is copy-pasta from the OP, so it was the same logical error there. And then, every `i` inside the loop should be a `Ptext[i]` ... – Aug 05 '17 at 01:13
Right, I fixed it but the answer was primarily to address the issue of the `Segmentation Fault`. – Felix Guo Aug 05 '17 at 01:15
Yes, it's hard to answer a question with code containing *so many* errors. Don't worry, I reall think it's a good answer as it addresses and explains the *main* problem .. it's just there are *other* problems as well... – Aug 05 '17 at 01:15

caesar ciphering (segmentation fault)

1 Answers1