Unsure as to why toupper() is cutting off last letter in C

Question

So the goal of this program is to basically take a 26 letter 'key' in the terminal (through argv[]) and use its index's as a substitution guideline. So there are 2 inputs you enter in the terminal, one in the argv[] and one is just a plain get_string() input. The argv[] input will look like this: ./s YTNSHKVEFXRBAUQZCLWDMIPGJO where s is the file name. And then the get_string() input will look like this: plaintext: HELLO. (The input is HELLO). What the program will then do is loop through all the letters in the plaintext input and substitute its alphabetical index according to the index of the argv[] key. For example, H has an alphabetical index of 7 (where a = 0 and z = 25), so we look at the 7th index in the key YTNSHKV(E)FXRBAUQZCLWDMIPGJO which in this case is E. It does this for each letter in the input and we'll end up with the output ciphertext: EHBBQ. This is what it should look like in the terminal:

./s YTNSHKVEFXRBAUQZCLWDMIPGJO
plaintext:  HELLO
ciphertext: EHBBQ

But my output is EHBB, since it cuts off the last letter for some reason when I use toupper().

And also, the uppercase and lowercase depends on the plaintext input, if the plaintext input was hello, world and the argv[] key was YTNSHKVEFXRBAUQZCLWDMIPGJO, the output would be jrssb, ybwsp, and if the input was HellO, world with the same key, the output would be JrssB, ybwsp.

I'm basically done with the problem, my program substitutes the plaintext given into the correct ciphertext based on the key that was inputted through the command line. Right now, say if the plaintext input was HELLO, and the key was vchprzgjntlskfbdqwaxeuymoi (all lowercase), then it should return HELLO and not hello. This is because my program puts all the letters in the command line key into an array of length 26 and I loop through all the plaintext letters and match it's ascii value (minus a certain number to get it into 0-25 index range) with the index in the key. So E has an alphabetical index of 4 so in this case my program would get lowercase p, but I need it to be P, so that's why I'm using toupper().

When I use tolower(), everything worked fine, and once I started using toupper(), the last letter of the ciphertext is cut off for some reason. Here is my output before using toupper():

ciphertext: EHBBQ

And here is my output after I use toupper():

ciphertext: EHBB

Here is my code:

int main(int argc, string argv[]) {
    string plaintext = get_string("plaintext: ");
    
    // Putting all the argv letters into an array called key
    char key[26]; // change 4 to 26
    for (int i = 0; i < 26; i++) // change 4 to 26
    {
        key[i] = argv[1][i];
    }
    
    // Assigning array called ciphertext, the length of the inputted text, to hold cipertext chars
    char ciphertext[strlen(plaintext)];
    
    // Looping through the inputted text, checking for upper and lower case letters
    for (int i = 0; i < strlen(plaintext); i++)
    {
        // The letter is lower case
        if (islower(plaintext[i]) != 0)
        {
            int asciiVal = plaintext[i] - 97; // Converting from ascii to decimal value and getting it into alphabetical index (0-25)
            char l = tolower(key[asciiVal]); // tolower() works properly
            //printf("%c", l);
            strncat(ciphertext, &l, 1); // Using strncat() to append the converted plaintext char to ciphertext
        }
        // The letter is uppercase
        else if (isupper(plaintext[i]) != 0)
        {
            int asciiVal = plaintext[i] - 65; // Converting from ascii to decimal value and getting it into alphabetical index (0-25)
            char u = toupper(key[asciiVal]);  // For some reason having this cuts off the last letter 
            strncat(ciphertext, &u, 1); // Using strncat() to append the converted plaintext char to ciphertext
        }
        // If its a space, comma, apostrophe, etc...
        else
        {
            strncat(ciphertext, &plaintext[i], 1);
        }
    }
    
    // prints out ciphertext output
    printf("ciphertext: ");
    for (int i = 0; i < strlen(plaintext); i++)
    {
        printf("%c", ciphertext[i]);
    }
    printf("\n");
    printf("%c\n", ciphertext[1]);
    printf("%c\n", ciphertext[4]);
    //printf("%s\n", ciphertext);
    return 0;
}

Answer 1

The strncat function expects its first argument to be a null terminated string that it appends to. You're calling it with ciphertext while it is uninitialized. This means that you're reading unitialized memory, possibly reading past the end of the array, triggering undefined behavior.

You need to make ciphertext an empty string before you call strncat on it. Also, you need to add 1 to the size of this array to account for the terminating null byte on the completed string to prevent writing off the end of it.

char ciphertext[strlen(plaintext)+1];
ciphertext[0] = 0;

Answer 2

There are multiple problems in the code:

• you do not test the command line argument presence and length
• the array should be allocated with 1 extra byte for the null terminator and initialized as an empty string for strncat() to work properly.
• instead of hard coding ASCII values such as 97 and 65, use character constants such as 'a' and 'A'
• strncat() is overkill for your purpose. You could just write ciphertext[i] = l; instead of strncat(ciphertext, &l, 1)
• islower() and isupper() are only defined for positive values of the type unsigned char and the special negative value EOF. You should cast char arguments as (unsigned char)c to avoid undefined behavior on non ASCII bytes on platforms where char happens to be a signed type.
• avoid redundant tests such as islower(xxx) != 0. It is more idiomatic to just write if (islower(xxx))

Here is a modified version:

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

int main(int argc, string argv[]) {
    // Testing the argument
    if (argc < 2 || strlen(argv[1]) != 26) {
        printf("invalid or missing argument\n");
        return 1;
    }
    // Putting all the argv letters into an array called key
    char key[26];
    memcpy(key, argv[1], 26);
    
    string plaintext = get_string("plaintext: ");
    int len = strlen(plaintext);
    
    // Define an array called ciphertext, the length of the inputted text, to hold ciphertext chars and a null terminator
    char ciphertext[len + 1];
    
    // Looping through the inputted text, checking for upper and lower case letters
    for (int i = 0; i < len; i++) {
        unsigned char c = plaintext[i];

        if (islower(c)) {        // The letter is lower case
            int index = c - 'a'; // Converting from ascii to decimal value and getting it into alphabetical index (0-25)
            ciphertext[i] = tolower((unsigned char)key[index]);
        } else
        if (isupper(c)) {
            // The letter is uppercase
            int index = c - 'A'; // Converting from ascii to decimal value and getting it into alphabetical index (0-25)
            ciphertext[i] = toupper((unsigned char)key[index]);
        } else {
            // other characters are unchanged
            ciphertext[i] = c;
        }
    }
    ciphertext[len] = '\0';  // set the null terminator

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