C Heap buffer corruption when using free()

Question

I'm getting the following error when freeing "shifted_text" below. I've checked with print statements and commenting things out, and it's definitely that free(shifted_text). The other free commands are working fine.

Debug Error! HEAP CORRUPTION DETECTED: After normal block (#77) at 0x007D1F...

CRT detected that the application wrote to memory after end of heap buffer.

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

void parse(int argc, char *argv[]);
char * shift_text(char *sometext);
char shift_letter(char letter, char shift);
void bring_in_text(void);
void write_to_file(char *sometext);

char *flag;
char *password;
char *text;

int main(int argc, char *argv[])
{
    parse(argc, argv);
    char *shifted_text;
    // at this point flag can only be -e, -d, -df, -ef
    if (strcmp(flag, "-e") == 0 || strcmp(flag, "-d") == 0)
    {
        // location to store the shifted text
        shifted_text = (char*)malloc(strlen(text) * sizeof(char));
        shift_text(shifted_text);
        printf("%s\n", shifted_text);
    }
    else
    {
        bring_in_text();
        // location to store the shifted text
        shifted_text = (char*)malloc(strlen(text) * sizeof(char));
        shift_text(shifted_text);
        write_to_file(shifted_text);
    }
    free(shifted_text);
    free(text);
    free(flag);
    free(password);
    return 0;
}

void write_to_file(char *sometext)
{
    if (strcmp(flag, "-df") == 0)
    {
        FILE *fp;
        fp = fopen("plaintext.txt", "w");
        if (fp == NULL)
        {
            puts("Unable to open file");
            exit(1);
        }
        fprintf(fp, sometext);
        fclose(fp);
    }
    else if (strcmp(flag, "-ef") == 0)
    {
        FILE *fp;
        fp = fopen("ciphertext.txt", "w");
        if (fp == NULL)
        {
            puts("Unable to open file");
            exit(1);
        }

        fprintf(fp, sometext);
        fclose(fp);
    }
}

void bring_in_text(void)
{
    if (strcmp(flag, "-df") == 0)
    {       
        FILE *fp;
        fp = fopen("ciphertext.txt", "r");
        if (fp == NULL)
        {
            puts("Unable to open file");
            exit(1);
        }
        while (!feof(fp))
        {
            text = (char*)malloc(100 * sizeof(char));
            fgets(text, 100, fp);
        }
        fclose(fp);
    }
    else if (strcmp(flag, "-ef") == 0)
    {
        FILE *fp;
        fp = fopen("plaintext.txt", "r");
        if (fp == NULL)
        {
            puts("Unable to open file");
            exit(1);
        }
        while (!feof(fp))
        {
            text = (char*)malloc(100 * sizeof(char));
            fgets(text, 100, fp);
        }
        fclose(fp);
    }

}


char * shift_text(char *shifted_text)
{
    char *temptext;
    temptext = text;
    char *tempshiftedtext;
    tempshiftedtext = shifted_text;
    // space for 10 characters plus null
    char *temppswd;
    temppswd = password;

    for (int i = 0; i < strlen(text); i++)
    {
        char a;
        if (*temptext >= 97 && *temptext <= 122)
        {
            a = shift_letter(*(temptext + i), *(temppswd + (i % strlen(password))));
            *(tempshiftedtext + i) = a;
        }
        else
            *(tempshiftedtext + i) = *(temptext + i);
    }
    *(tempshiftedtext + strlen(text)) = '\0';
}

char shift_letter(char letter, char shift)
{
    if (strcmp(flag, "-e") == 0 || strcmp(flag, "-ef") == 0)
    {
        letter = letter - 97;
        shift = shift - 97;
        int shifted_letter = letter + shift;
        if (shifted_letter > 25)
            shifted_letter %= 26;
        shifted_letter += 97;
        return (char)shifted_letter;
    }
    else if (strcmp(flag, "-d") == 0 || strcmp(flag, "-df") == 0)
    {
        int shifted_letter = letter - 97;
        shift = shift - 97;
        int letter = shifted_letter - shift;

        letter %= 26;   // mod seems to allow negative results, so if its still negative. add another val equal to modulus
        if (letter < 0)
            letter += 26;
        letter += 97;
        return (char)letter;
    }
}

void parse(int argc, char *argv[])
{
    if (argc == 4)
    {
        // internally calls malloc on strlen(argv[i])
        flag = _strdup(argv[1]);
        password = _strdup(argv[2]);
        text = _strdup(argv[3]);
        if (strlen(password) > 10)
        {
            puts("Password too long");
            exit(1);
        }
        else if (strcmp(flag, "-e") != 0 && strcmp(flag, "-d") != 0)
        {
            puts("Incorrect flag");
            exit(1);
        }
    }
    else if (argc == 3)
    {
        // internally calls malloc on strlen(argv[i])
        flag = _strdup(argv[1]);
        password = _strdup(argv[2]);
        if (strlen(password) > 10)
        {
            puts("Password too long");
            exit(1);
        }
        else if (strcmp(flag, "-ef") != 0 && strcmp(flag, "-df") != 0)
        {
            puts("Incorrect flag");
            exit(1);
        }
    }
    else
    {
        puts("Incorrect arguements");
        exit(1);
    }
}

The functions parse simply stores command line arguments in the global's. The shifting functions shift a letter by some number. 'A' shifted by 2 would be 'C' for example. These work fine and without the free(shifted_text) the program works.

I'm new to C so it's probably something simple but I can't see it.

Without seeing what's happening inside `shift_text` function how could we help you? — Jack, Mar 20 '16 at 00:17
You do know that strings in C have an extra terminator character? So the actual length of e.g. `text` in your code is `strlen(text) + 1`. — Some programmer dude, Mar 20 '16 at 00:18
Oh and on an unrelated note, the C specification says that `sizeof(char)` will *always* be equal to `1`. — Some programmer dude, Mar 20 '16 at 00:24
@JoachimPileborg Yes I know, I do sizeof(char) purely for consistency, but thank you — John Setter, Mar 20 '16 at 00:25
`*temptext >= 97 && *temptext <= 122` you should compare to `char` literals — lost_in_the_source, Mar 20 '16 at 00:26
Then another couple of unrelated notes: Please read [Why is “while ( !feof (file) )” always wrong?](http://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong). And don't use [magic numbers](https://en.wikipedia.org/wiki/Magic_number_%28programming%29), especially when C have some fine [character classification functions](http://en.cppreference.com/w/c/string/byte#Character_classification). — Some programmer dude, Mar 20 '16 at 00:29
@JoachimPileborg do you recommend checking for EOF instead of !feof(file)? And its a nice comment about using the isalpha function, that's what they're there for after all! Thanks! — John Setter, Mar 20 '16 at 00:35
Is [`valgrind`](http://valgrind.org/) available on your platform? If so, use it. But a primary problem is your mishandling of string lengths, not allowing for the null terminator. Are you working on Windows? The `_strdup()` function is non-standard except perhaps on Windows where the creators of the runtime seem to suffer from delusions about the POSIX standard (few POSIX names start with an underscore, and specifically, `strdup()` does not). However, you really don't need to make copies of the program arguments; just set the pointers to point at the program arguments. — Jonathan Leffler, Mar 20 '16 at 00:36
@JonathanLeffler Yes I'm using Visual Studio 2015 community on Win7 — John Setter, Mar 20 '16 at 00:37
`shift_text` never returns a value, and `shift_letter` doesn't return a value for all paths of execution. — Michael Albers, Mar 20 '16 at 00:38
The code in `bring_in_text()` like this: `while (!feof(fp)) { text = (char*)malloc(100 * sizeof(char)); fgets(text, 100, fp); }` leaks memory like the bottom's falling out of the stock market. You need to rethink what you're doing there (both branches). — Jonathan Leffler, Mar 20 '16 at 00:38
Since you *always* allocate a fixed size for `text` I don't see why you're not making it an array? Then you can simply use `while (fgets(...) != NULL))`. But I don't really understand what the loop is for? What is the contents of those files? Is it only a single line (why do you need the loop for then?), otherwise you will only get the last line (*and* with the current code have a massive memory leak). — Some programmer dude, Mar 20 '16 at 00:38
Thanks guys, pretty stupid of me to malloc inside a loop.... — John Setter, Mar 20 '16 at 00:41
So does anyone have any idea why I'm still getting the heap corruption when freeing shifted_text? — John Setter, Mar 20 '16 at 00:53
shift_text function has a type of char * , but doesn't return anything. — Arif Burhan, Mar 20 '16 at 01:04

score 1 · Answer 1 · answered Mar 20 '16 at 03:36

Change this

shifted_text = (char*)malloc(strlen(text) * sizeof(char));

to

shifted_text = malloc((strlen(text) + 1) * sizeof(char)); // don't cast

A C-style string always has a null-terminator, indicating the end of the string. For example, "foo" is stored as 'f', 'o', 'o', '\0' in memory. So you have to

I suspect that the heap buffer corruption isn't caused by your free(shifted_text);. Since insufficient memory is allocated to shifted_text, undefined behaviour is invoked, making everything possible. So your program may either run properly or crash. Perhaps it's only a coincidence that every time free(shifted_text); is commented out, your program runs correctly thanks to the undefined behaviour.

BTW: There are many places in your code to be refined. For example, in void bring_in_text(void):

while (!feof(fp))
{
    text = (char*)malloc(100 * sizeof(char));
    fgets(text, 100, fp);
}

Covering the previous lines without even processing them? Also, text isn't freed in this function.

score 0 · Answer 2 · answered Mar 20 '16 at 01:05

0

strdup allocates strlen+1 chars and you only allocate strlen chars. When you write the null at the end of shifted text you are overflowing the buffer.

answered Mar 20 '16 at 01:05

jkuz

131
1
5

C Heap buffer corruption when using free()

2 Answers2