-1
\$\begingroup\$

The program is a Caesar's cipher decoder. It compiles and works ok, but in Valgrind, I receive a lot of errors and I have no idea how to solve them.

I have tried with --track-origins=yes, but I still have no clue what's wrong.

Example to test: xUbbemehbT, XYlloworld -> Helloworld.

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

#define MODE_1 1
#define MODE_2 2

enum Error
{
    ERROR_INPUT = 100,
    ERROR_LENGTH = 101
};

enum Msg
{
    DEFAULT_LEN = 10,
    LAST_LOWER = 26,
    LAST_UPPER = 52
};

int mandatory(void);
int optional(void);

void sub_shift(char *message, char mode);
char *shift(char *original, char *misheard);
int compute_shift(char *original, char *misheard, size_t len, int shift_1);
int compare_by_symbol(const char *msg1, const char *msg2, int length);
int free_and_exit(char *msg1, char *msg2, int error);

int main(int argc, char *argv[])
{
    int ret = EXIT_SUCCESS;

    bool prp_optional = argc > 1 ? strcmp(argv[1], "-prp-optional") == 0 : false;

    prp_optional ? optional(), ret = optional() : mandatory(), ret = mandatory();

    return ret;
}

void sub_shift(char *message, char mode)
{
    size_t len = strlen(message) - 1; // -1 Because of '\0'
    if (mode == MODE_1)
    {
        for (size_t letter = 0; letter < len; letter++)
        {
            if (message[letter] > 'Z' + 1)
            {
                message[letter] -= 'G';
            }
            else
            {
                message[letter] -= 'A';
            }
        }
    }
    else
    {
        for (size_t letter = 0; letter < len; letter++)
        {
            if (message[letter] > LAST_LOWER)
            {
                message[letter] += 'G';
            }
            else
            {
                message[letter] += 'A';
            }
        }
    }
}

char *shift(char *original, char *misheard)
{
    int shift_1 = 0;
    size_t len = strlen(original) - 1; // -1 Because of '\0'

    sub_shift(original, MODE_1);
    sub_shift(misheard, MODE_1);

    shift_1 = compute_shift(original, misheard, len, shift_1);
    
    for (size_t letter = 0; letter < len; letter++)
    {
        original[letter] += shift_1;
        original[letter] %= LAST_UPPER;
    }

    sub_shift(original, MODE_2);
    original[len] = '\0'; // Remove the last character

    return original;
}

int compute_shift(char *original, char *misheard, size_t len, int shift_1)
{
    int off = 0;
    for (int index = 0; index <= LAST_UPPER; index++)
    {
        int offset = 0;
        for (size_t letter = 0; letter < len; letter++)
        {
            original[letter]++;
            original[letter] %= LAST_UPPER;

            if (original[letter] == misheard[letter])
            {
                offset++;
            }
        }
        if (off < offset)
        {
            off = offset;
            shift_1 = index;
        }
    }

    return shift_1;
}

int compare_by_symbol(const char *msg1, const char *msg2, int length)
{
    int equal_counter = 0;
    for (int i = 0; i < length; i++)
    {
        if (msg1[i] == msg2[i])
        {
            equal_counter++;
        }
    }
    return equal_counter;
}

int free_and_exit(char *msg1, char *msg2, int error)
{
    free(msg1);
    free(msg2);
    msg1 = NULL;
    msg2 = NULL;
    if (error == ERROR_INPUT)
    {
        fprintf(stderr, "Error: Wrong input!\n");
        exit(ERROR_INPUT);
    }
    else if (error == ERROR_LENGTH)
    {
        fprintf(stderr, "Error: Wrong input length!\n");
        exit(ERROR_LENGTH);
    }
    else
    {
        free(msg1);
        free(msg2);
        msg1 = NULL;
        msg2 = NULL;
        exit(EXIT_SUCCESS);
    }
}

int mandatory(void)
{
    int ret = EXIT_SUCCESS;

    size_t capacity = DEFAULT_LEN;
    int positions[LAST_UPPER];

    size_t len1 = 0;
    size_t len2 = 0;

    char *msg1 = (char *)malloc(capacity);
    char *msg2 = (char *)malloc(capacity);

    while ((msg1[len1] = getchar()) != EOF && msg1[len1] != '\n')
    {
        // If is not in an alphabet (not a letter a-zA-Z)
        if (!isalpha(msg1[len1]))
        {
            ret = free_and_exit(msg1, msg2, ERROR_INPUT);
        }
        // Extend if exceed capacity
        if (++len1 == capacity)
        {
            msg1 = (char *)realloc(msg1, capacity * 2);
        }
    }
    while ((msg2[len2] = getchar()) != EOF && msg2[len2] != '\n')
    {
        // If is not in an alphabet (not a letter a-zA-Z)
        if (!isalpha(msg2[len2]))
        {
            ret = free_and_exit(msg1, msg2, ERROR_INPUT);
        }
        // Extend if exceed capacity
        if (++len2 == capacity)
        {
            msg2 = (char *)realloc(msg2, capacity * 2);
        }
    }
    if (len1 != len2)
    {
        ret = free_and_exit(msg1, msg2, ERROR_LENGTH);
    }

    char *msg_temp = (char *)malloc(len1);

    for (int offset = 0; offset < LAST_UPPER; offset++)
    {
        strcpy(msg_temp, msg1);

        positions[offset] = compare_by_symbol(msg_temp, msg2, len2);
    }

    int last_offset = 0;
    for (int i = 0; i < LAST_UPPER; i++)
    {
        if (positions[i] > last_offset)
        {
            last_offset = i;
        }
    }
    // Final decode
    msg1 = shift(msg1, msg2);

    // Print decoded message
    printf("%s\n", msg1);

    free(msg_temp);
    msg_temp = NULL;
    ret = free_and_exit(msg1, msg2, EXIT_SUCCESS);

    return ret;
}

int optional(void)
{
    /* Same as mandatory, only length comparison check is removed */
    int ret = EXIT_SUCCESS
    size_t capacity = DEFAULT_LEN;
    int positions[LAST_UPPER];

    size_t len1 = 0;
    size_t len2 = 0;

    char *msg1 = (char *)malloc(capacity);
    char *msg2 = (char *)malloc(capacity);

    while ((msg1[len1] = getchar()) != EOF && msg1[len1] != '\n')
    {
        // If is not in an alphabet (not a letter)
        if (!isalpha(msg1[len1]))
        {
            ret = free_and_exit(msg1, msg2, ERROR_INPUT);
        }
        // Extend if exceed capacity
        if (++len1 == capacity)
        {
            msg1 = (char *)realloc(msg1, capacity * 2);
        }
    }
    while ((msg2[len2] = getchar()) != EOF && msg2[len2] != '\n')
    {
        // If is not in an alphabet (not a letter)
        if (!isalpha(msg2[len2]))
        {
            ret = free_and_exit(msg1, msg2, ERROR_INPUT);
        }
        // Extend if exceed capacity
        if (++len2 == capacity)
        {
            msg2 = (char *)realloc(msg2, capacity * 2);
        }
    }

    char *msg_temp = (char *)malloc(len1);

    for (int offset = 0; offset < LAST_UPPER; offset++)
    {
        strcpy(msg_temp, msg1);

        positions[offset] = compare_by_symbol(msg_temp, msg2, len2);
    }

    int last_offset = 0;
    for (int i = 0; i < LAST_UPPER; i++)
    {
        if (positions[i] > last_offset)
        {
            last_offset = i;
        }
    }
    // Final decode
    msg1 = shift(msg1, msg2);

    // Print decoded message
    printf("%s\n", msg1);

    free(msg_temp);
    msg_temp = NULL;
    ret = free_and_exit(msg1, msg2, EXIT_SUCCESS);

    return ret;
}

EDIT: Valgrind output.

==1308== Memcheck, a memory error detector
==1308== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==1308== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==1308== Command: ./main
==1308== \ xUbbemehbT\ XYlloworld
==1308== Conditional jump or move depends on uninitialised value(s)
==1308== at 0x483F0B7: strcpy (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1308== by 0x10BAB2: strcpy (string_fortified.h:90)
==1308== by 0x10BAB2: mandatory (main.c:214)
==1308== by 0x1091CC: main (main.c:38)
==1308== Uninitialised value was created by a heap allocation
==1308== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1308== by 0x10B9B7: mandatory (main.c:176)
==1308== by 0x1091CC: main (main.c:38)
==1308==
==1308== Invalid write of size 1
==1308== at 0x483F0BE: strcpy (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1308== by 0x10BAB2: strcpy (string_fortified.h:90)
==1308== by 0x10BAB2: mandatory (main.c:214)
==1308== by 0x1091CC: main (main.c:38)
==1308== Address 0x4a495eb is 0 bytes after a block of size 11 alloc'd
==1308== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1308== by 0x10BA9F: mandatory (main.c:210)
==1308== by 0x1091CC: main (main.c:38)
==1308==
==1308== Conditional jump or move depends on uninitialised value(s)
==1308== at 0x483EF58: strlen (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1308== by 0x10983F: shift (main.c:79)
==1308== by 0x10BAC2: mandatory (main.c:228)
==1308== by 0x1091CC: main (main.c:38)
==1308== Uninitialised value was created by a heap allocation
==1308== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1308== by 0x10B9B7: mandatory (main.c:176)
==1308== by 0x1091CC: main (main.c:38)
==1308==
==1308== Conditional jump or move depends on uninitialised value(s)
==1308== at 0x483EF58: strlen (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1308== by 0x109AB8: sub_shift (main.c:45)
==1308== by 0x109AB8: shift (main.c:82)
==1308== by 0x10BAC2: mandatory (main.c:228)
==1308== by 0x1091CC: main (main.c:38)
==1308== Uninitialised value was created by a heap allocation
==1308== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1308== by 0x10B9C4: mandatory (main.c:177)
==1308== by 0x1091CC: main (main.c:38)
==1308==
==1308== Conditional jump or move depends on uninitialised value(s)
==1308== at 0x483EF58: strlen (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1308== by 0x10AA4E: sub_shift (main.c:45)
==1308== by 0x10AA4E: shift (main.c:92)
==1308== by 0x10BAC2: mandatory (main.c:228)
==1308== by 0x1091CC: main (main.c:38)
==1308== Uninitialised value was created by a heap allocation
==1308== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1308== by 0x10B9B7: mandatory (main.c:176)
==1308== by 0x1091CC: main (main.c:38)
==1308== \ Helloworld
==1308==
==1308== HEAP SUMMARY:
==1308== in use at exit: 0 bytes in 0 blocks
==1308== total heap usage: 5 allocs, 5 frees, 2,261 bytes allocated
==1308==
==1308== All heap blocks were freed -- no leaks are possible
==1308==
==1308== For lists of detected and suppressed errors, rerun with: -s
==1308== ERROR SUMMARY: 107 errors from 5 contexts (suppressed: 0 from 0)

\$\endgroup\$
10
  • 1
    \$\begingroup\$ size_t len = strlen(message) - 1; // -1 Because of '\0' That doesn't make sense, since strlen does not count the terminator. \$\endgroup\$
    – JDługosz
    Nov 17 '21 at 16:41
  • \$\begingroup\$ Here's one problem: char *msg_temp = (char *)malloc(len1); It appears len1 is the count of characters, and does not include room for the terminator. \$\endgroup\$
    – JDługosz
    Nov 17 '21 at 16:44
  • \$\begingroup\$ Please tell us more about the purpose of the code. \$\endgroup\$
    – Mast
    Nov 17 '21 at 16:45
  • \$\begingroup\$ Code decodes first input based on how it is similar to the second input which is changed original message. Thanks JDlugosz, now there are a bit less errors \$\endgroup\$
    – sznailc
    Nov 17 '21 at 16:51
  • 2
    \$\begingroup\$ @sznailc given your update from 21 hours ago, Are you open to feedback about any or all facets of the code? If so, then please edit the post to contain the working code (with errors removed). \$\endgroup\$ Nov 18 '21 at 18:11
1
\$\begingroup\$
    /* Same as mandatory, only length comparison check is removed */

Don't do that. You duplicated the entire (complex) function to make minor changes; specifically to skip error checks.

Write one copy of the function. Put the optional checks in another preliminary function that's called first, or pass a boolean parameter to enable/disable the tests.

You might reconsider whether you want a version that doesn't do error checking. Are these tests super expensive compared to the work being done?

\$\endgroup\$
1
  • \$\begingroup\$ I'll change it, but it is not the main problem now \$\endgroup\$
    – sznailc
    Nov 17 '21 at 16:53
1
\$\begingroup\$

Avoid subtraction with unsigned math

When message[0] == 0 and size_t len = strlen(message) - 1;, len takes on the value of SIZE_MAX. Instead:

void sub_shift(char *message, char mode) {
  // size_t len = strlen(message) - 1;
  size_t len = strlen(message);
  if (mode == MODE_1)
  {
    // for (size_t letter = 0; letter < len; letter++)
    for (size_t letter = 0; letter + 1 < len; letter++)

Use consistent types

length deserves to be size_t to match calling code and for the best type to use with string lengths.

size_t compare_by_symbol(const char *msg1, const char *msg2, size_t length) {
  size_t equal_counter = 0;
  for (size_t  i = 0; i < length; i++) {
    if (msg1[i] == msg2[i]) {
      equal_counter++;
    }
  }
  return equal_counter;
}
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.