Why do i get "output not valid ASCII text" even though my printf prints the right result?

Question

I am making the substitution project for CS50 (Week 2, problem set 2).

The code seems to work fine and I get the answer I want when I test the code. However the code only seems to work for single letters. All the other tests get "output not valid ASCII text".

I have tried to printf(ciphertext: ) with charachters printed individually. Didnt help.

I have tried moving my result[stringLength] = '\0'; (Move it to the end of the code, also tried to redefine it after the last letter was pasted into the array). Didnt change anything.

When I tried to research this question a bunch of different answers came up but mostly you need "= '\0'" which did help but only for the single letter answers.

I have also tried using the char array as a string (seemed to work for a few other people), but it didn't change anything and it still only worked for single letters.

The code:

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

int main(int argc, string argv[])
{
    if (argc < 2)
    {
        printf("Usage: ./substitution key\n");
        return 1;
    }
    else if (argc > 2)
    {
        printf("Usage: ./substitution key\n");
        return 1;
    }
    else if (strlen(argv[1]) != 26)
    {
        printf("Key must contain 26 characters.\n");
        return 1;
    }
    int argv_length = strlen(argv[1]);
    printf("%i length\n", argv_length);
    printf("%i argc (argumenter)\n", argc);
    printf("%s cipher\n", argv[1]);
    string input = get_string("plaintext: ");
    int stringLength = strlen(input);
    printf("%i string length\n", stringLength);
    char result[stringLength];
    result[stringLength] = '\0';

    for (int n = 0; n < strlen(input); n++)
    {
        int u = 0;
        if (islower(input[n]) != 0)
        {
            u = input[n] - 97;
        }
        else
        {
            u = input[n] - 65;
        }
        printf("%c Input N\n", input[n]);
        char output1 = argv[1][u];
        if (isalpha(input[n]) == 0)
        {
            output1 = input[n];
        }
        printf("%c, OUTPUT1\n", output1);
        int toLowerCheck = islower(input[n]);
        printf("%i islower?\n", toLowerCheck);
        if (islower(input[n]) == 0)
        {
            printf("I am lowering");
            output1 = toupper(output1);
        }
        else
        {
            output1 = tolower(output1);
        }
        printf("%c, OUTPUT1 after lowering/uppering\n", output1);
        result[n] = output1;
        printf("%s letter\n", result);
    }
    printf("ciphertext: ");
    for (int k = 0; k < strlen(result); k++)
    {
        printf("%c", result[k]);
    }
    printf("\n");
    //printf("ciphertext: %s\n", result);

    return 0;
}

Answer 1

You have an off-by-one error.

int stringLength = strlen(input);
char result[stringLength];
result[stringLength] = '\0';

C strings are terminated by a null byte. strlen returns the length of the string in bytes, but it does not include the null byte. result is too short by 1 byte. To store a 5 characters in a string you need 6 bytes.

Indexes start at 0. If your array is of size 5, its indexes are 0, 1, 2, 3, 4. result is of size stringLength so result[stringLength] = '\0' is 1 off the end of the array. It accesses memory which does not belong to result and so the behavior is undefined.

// incorrect
char result[5];    # result can store a 4 character string
result[5] = '\0';  # highest index is 4.

// correct
char result[6];    # result can store a 5 character string
result[5] = '\0';  # highest index is 5.

Something else later might overwrite that null byte. C uses that null byte to know where the string ends; anything reading result will start reading adjacent memory and result will seem to be longer than it is.

To fix it, allocate one more byte for the null.

int stringLength = strlen(input);
char result[stringLength+1];

---

You have a second problem here:

result[n] = output1;
printf("%s letter\n", result);

You never initialize result, you only put the null byte at the end. The rest of result is uninitialized. You're reading whatever happens to be in memory. This "works" because often memory happens to be 0, and printf will stop reading at 0. But if it wasn't 0 you'd get a bunch of garbage.

For example. Let's say you have 5 bytes. When you allocate result it contains some trash.

char result[stringLength+1];

// result: {'t', 'r', 'a', 's', 'h', '!'};

result[stringLength] = '\0';

// result: {'t', 'r', 'a', 's', 'h', '\0'};

Now you start looping, assigning to result, and printing result.

result[0] = 'a'`

// result: {'a', 'r', 'a', 's', 'h', '\0'};

printf("%s letter\n", result);  // Prints "arash"

If you want to print result as you loop, you need to make sure it is initialized.

// This also precludes the need to set the null byte manually
// because the whole string is full of nulls.
char result[stringLength+1];
memset(result, '\0', stringLength);

In general, initialize every variable.