CS50 Vigènere problem. Out of bounds access, undefined behavior

Question

So the this is the solution i came with for the CS50 Vigenère problem; I'm fairly new to programming, like only a few weeks so i'm sorry in advance for the form of my code.

The problem here is that the output isn't what i expect it to be.

examples:

./vigenere ABC

input: hello

output: hfnLp

./vigenere ABC

input: HELLO

output: HFN,P

./vigenere BACON

input: Meet me at the park at eleven am

output: Neg zF av uf pCx bT gzrwEP OZ

(it's supposed to be "Negh zf av huf pcfx bt gzrwep oz")

i'm clueless, because it seems to be working a little bit, but somethings off.

I checked almost every single integer that i'm using, all of them act as i intended them to.

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

int main (int argc, string argv[])
{
    string k = argv[1]; //key
    string p; //plaintext
    int j = 0; //the counter for key index
    int ci; // the variable for reaching the right shifting value on both uppercase and lowercase letters
    int K;//the value of the key index shift

    if (argc != 2)
    {
        printf("input error\n");
    }
    else
    {
       p = get_string();

       for (int i = 0, n = strlen(p); i < n; i++)
       {
           if (isupper(k[j]))
           {
            ci = 65;
           }
           else
           {
            ci = 97;
           }

           K = k[j % strlen(k)] - ci;

           if (isalpha (p[i]))
           {
                printf("%c", p[i] + K);
                j++;
           }
           else
           {
            printf("%c", p[i]);
           }
       }
       printf("\n");
    }
}

Out of bounds access, undefined behavior. – EOF Apr 03 '19 at 10:41 — EOF, Apr 03 '19 at 10:41

Eric Postpischil · Accepted Answer · 2019-04-03T10:48:42.460

1

After strlen(k) iterations, isupper(k[j]) uses an index beyond the end of k.

You could change:

if (isupper(k[j]))
{
 ci = 65;
}
else
{
 ci = 97;
}

K = k[j % strlen(k)] - ci;

to:

K = toupper(k[j % strlen(k)]) - 'A';

(Note this relies on a property not guaranteed by the C standard, that the character codes for letters are consecutive and in alphabetical order.)

edited Apr 03 '19 at 10:48

answered Apr 03 '19 at 10:43

Eric Postpischil

195,579
13
168
312

Hello! Thank you for the answer, it helped me out alot. I noticed that i could make the code work properly if i replaced ```if (isupper(k[j]))``` by ```if(isupper(k[j % strlen(k)])```, but i went with the way you told me to. I wanted to ask about the _strlen_ function. Wouldn't it be better (memory wise) to declare an integer that holds the ```strlen(k)``` value instead of calling it over and over again? – Ilias Timaev Apr 04 '19 at 23:16
@IliasTimaev: Sure, it is good to evaluate `strlen(k)` once and cache it for later use. Compilers are good enough these days that they might recognize `k` is not changed in a loop (and they know what `strlen` does), so they might do that for you when optimization is enabled. But it is good to put it in the code explicitly. – Eric Postpischil Apr 04 '19 at 23:59

CS50 Vigènere problem. Out of bounds access, undefined behavior

1 Answers1