-1

So I'm on week 2 of the harvard cs50 online course (2021x version). We're supposed to write a program that encrypts a text, moving every letters ASCII code by a certain amount, decided by the user through the command line. Here is the full problem. I'm nearly done, but already when I try to run the program, (it compiles just fine), it tells me there is a segmentation error. I don't really understand the problem. I've read that the problem has to do with accessing a certain part of memory that isn't accessible?? How do I solve this problem? Thank you in advance! Here is my code..

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

int main(int argc, string argv[])
{
    int k, i;
    if (argc == 2 && isdigit(argv[1]) && argv[1] > 0)
    {
        k = (int) argv[1];
        string plaintext = get_string("plaintext: ");
        printf("cyphertext: ");
        for (i = 0; i < strlen(plaintext); i++)
        {
            if (islower(plaintext[i]) || isupper(plaintext[i]))
            {
                printf("%c", (((plaintext[i] + k) - 97) % 26) + 97);
                return 0;
            }
            
        }
    }
    else
    {
        printf("Usage: ./caesar key");
        return 1;
    }
}
charlie
  • 3
  • 1
  • [Debugging is a useful skill, now would be a great time to try it.](https://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems) At least a debugger would give you a hint where the crash happens. –  Feb 18 '21 at 13:27
  • Can you show the declaration of `string` type ? `argv[1]` is a pointer which points a string, `(int) argv[1]` cannot get the decimal value of `argv[1]`, but the address of the string. Sorry for my poor English. – guapi Feb 18 '21 at 13:29
  • Other anomalies visible: `argv[1]>0` is not a well-defined operation (pointers again), `return 0` after the first letter has been processed looks a bit out of place too. –  Feb 18 '21 at 13:32
  • @guapi seems to be `typedef char *string;`, see https://github.com/cs50/libcs50/blob/develop/src/cs50.h –  Feb 18 '21 at 13:38
  • `isdigit(argv[1])` should be `isdigit(*argv[1])` (or `isdigit(argv[1][0]`). `k = (int) argv[1];` should be `k = atoi(argv[1]);` or something similar using `strtol`. – Ian Abbott Feb 18 '21 at 14:08

2 Answers2

1

I'm on week 2 of the harvard cs50 online course

I'm sorry to hear that. Most of your problems originate from the bad CS-50 class that tricks you into believing that C somehow has a pre-made, sugar-coated string class. It has not, it only got raw character arrays ending with null termination and most string manipulation is therefore rather manual.

Other problems come from not listening to the compiler when it points out bugs to you. Here's a few:

  • isdigit(argv[1]) doesn't make sense. A good compiler tells you as much, for example clang:

    warning: cast to smaller integer type 'int' from 'string' (aka 'char *')

    You compare a pointer to a string while isdigit expects a single character. In order to use isdigit you must call it in a loop for each character of the string.

  • argv[1] > 0 doesn't make sense because it compares a pointer against 0.

    error: ordered comparison between pointer and zero

  • k = (int) argv[1]; doesn't make sense either.

    warning: cast to smaller integer type 'int' from 'string' (aka 'char *')

    To convert from a string to integer, you must use the strtol function, for example k = strtol(argv[1], NULL, 10). It comes with various error handling that you might want to use too.

    And because k contains nonsense - actually it contains a memory address converted to integer, plaintext[i] + k becomes nonsense too.

  • if (islower(plaintext[i]) || isupper(plaintext[i])) doesn't make sense, it says "if a character is lower case or it is upper case". Well, it probably is? Rather, you probably meant to use toupper or tolower to do all calculations either on upper or lower case regardless of what the user typed.

Lundin
  • 195,001
  • 40
  • 254
  • 396
0

You should check the lower character and the upper character individually. If the character is lower character, you're right. You should subtract 97.But if the character is upper character you should subtract 65. Otherwise you get an negative result before the mode operation and this causes to the Segmentation fault.

if(isupper(plainText[i])){
                
                cipherText[i] = (((plainText[i] - 65 + key) % 26) + 65);
                
            } else {
if(islower(plainText[i])){
                
                cipherText[i] = (((plainText[i] - 97 + key) % 26) + 97);
                    
                }
            }
Ogün Birinci
  • 598
  • 3
  • 11
  • 1
    Nope, that's not it. It crashes in `isdigit`. –  Feb 18 '21 at 13:51
  • You're right, but is it also another problem? – Ogün Birinci Feb 18 '21 at 14:06
  • It's also a problem. Not a segfaulty one (assigning any int value into a char will do something well-defined), more "incorrect results" one. Anyway, suggesting 65 and 97 is not the best, how about 'a' and 'A' instead? –  Feb 18 '21 at 14:10
  • (cue the ebcdic crowd "b-b-but non-consecutive alphabet encodings") :) –  Feb 18 '21 at 14:11
  • thank you for suggesting this! i didnt even notice it, but it makes sense. like the other comments expected, it did not however solve the segfault. – charlie Feb 20 '21 at 12:36