2

I'm creating a Caesar cipher in C and I'm having an issue when displaying the encoded message. It works if the message consists of very few characters but as soon as the message exceeds a certain amount of characters the printf function begins to display unknown characters which I assume are junk bytes. Any ideas?

Code:

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

char *concat(const char *s1, const char *s2)
{
    /*
        Remember to free allocated memory
    */
    char *result;
    if((result = malloc(strlen(s1)+strlen(s2)+1)) != NULL)
    {
        result[0] = '\0';
        strcat(result, s1);
        strcat(result, s2);
    }

    return result;
}

void encode(const char *alpha, const char *message)
{
    char result[(sizeof(message) / sizeof(char))];
    int x, y, z;

    memset(result, '\0', sizeof(result));
    for(x = 0; x < strlen(message); x++)
    {
        for(y = 0; y < 25; y++)
        {
            if(alpha[y] == message[x])
            {
                z = (y + 3);
                if(z > 25)
                {
                    z = z % 25;
                }

                result[x] = alpha[z];
            }
        }
    }

    printf("%s\n", result);

    //return result;
}

int main()
{
    char message[200];
    char *result;
    char array[26] = { 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z' };

    printf("Input your message:");
    scanf("%s", &message);

    encode(array, message);
}

Result:

enter image description here

1 Answers1

2

Change

char result[(sizeof(message) / sizeof(char))];

to

char result[strlen(message) + 1];

This leaves room in your buffer for a terminating null-character. sizeof also can ont be used to get the length of c-strings, we need to use strlen for this. You will need to update the memset call as well.

sizeof is used by the compiler, so in this case you will ask your compiler "what is the size of message", which in this case will be 8 (assuimg 64-bit), because a pointer is 8 bytes.

Other than that you should be careful with scanf, it is fine as it is now because you read one line of input, but if you tried reading one more line you might run into trouble.

As mentioned in the comments, for(x = 0; x < strlen(message); x++) runs strlen once per loop iteration, adding a rather big extra load on your program, that grows exponentially as the length of message increases.

Your concat function is unused, unsure why you included it?

  • 1
    Ah, my fault for forgetting the allocated space for the null byte terminator, thank you. –  Nov 08 '17 at 10:38
  • @JosephHarris The null byte isn't the problem. The problem is you're taking the size of a pointer, which is probably 4 or 8 and completely independent of the how long the string is. – Tom Karzes Nov 08 '17 at 10:39
  • 1
    Ah so sizeof will only return the size of the actual pointer and in order to find the actual length of the string strlen is suitable. –  Nov 08 '17 at 10:41
  • I was tampering with it and then I realised it wasn't useful but hadn't yet removed it. –  Nov 08 '17 at 10:43
  • you might want to correct this memset as well memset(result, '\0', sizeof(result)); – asio_guy Nov 08 '17 at 10:43
  • 3
    `sizeof` is not evaluated by the preprocessor. It gets evaluated during the actual compilation. – Tom Karzes Nov 08 '17 at 10:45