Heap corruption from memory allocation using malloc: why did it happen?

Question

Ok, I was trying to implement memmove just as a programming exercise, and I get a memory access violation in the memmove function when I try to use malloc. Here is the function:

//Start

void* MYmemmove (void* destination, const void* source, size_t num) { 

    int* midbuf = (int *) malloc(num); // This is where the access violation happens.
    int* refdes = (int *) destination; // A pointer to destination, except it is casted to int*
    int* refsrc = (int *) source; // Same, except with source
    for (int i = 0;num >= i;i++) { 
        midbuf[i] = *(refsrc + i); // Copy source to midbuf
    }
    for (int i = 0;num >= i;i++) { 
        refdes[i] = *(midbuf + i); // Copy midbuf to destination
    } 
    free(midbuf); // free midbuf 
    refdes = NULL; // Make refdes not point to destination anymore
    refsrc = NULL; // Make refsrc not point to source anymore
    return destination;
}

By the way, I am sort of a newbie to pointers, so don't be suprised if there is some mistakes. What am I doing wrong?

Answer 1

Please be careful with the other suggestions! The answer depends on how your memmove will be used. The other answers state you should change your malloc call to account for the size of an int. However, if your memmove function will be used to mean "move this number of bytes" then the implementation would be wrong. I would instead go about using char* as that takes care of several problems in one go.

Also, an int is typically 4 bytes, and char is typically 1 byte. If the void* address you receive is not word aligned (not a multiple of 4 bytes), you will have a problem: To copy an int that is not word aligned you will have to do more than one read and expensive bit masking. This is inefficient.

Finally, the memory access violation happened because you were incrementing your midbuf int pointer each time, and moving forward 4 bytes at a time. However, you only allocated num bytes, and thus would eventually try to access past the end of the allocated region.

/** Moves num bytes(!) from source to destination */
void* MYmemmove (void* destination, const void* source, size_t num) { 

    // transfer buffer to account for memory aliasing
    // http://en.wikipedia.org/wiki/Aliasing_%28computing%29
    char * midbuf = (char *) malloc(num); // malloc allocates in bytes(!)
    char * refdes = (char *) destination;
    char * refsrc = (char *) source;

    for (int i = 0; i < num; i++) { 
        midbuf[i] = *(refsrc + i); // Copy source to midbuf
    }

    for (int i = 0; i < num; i++) { 
        refdes[i] = *(midbuf + i); // Copy midbuf to destination
    } 

    free(midbuf); // free midbuf
    // no need to set the pointers to NULL here.
    return destination;
}

By copying byte by byte, we avoid alignment issues, and cases where num itself could be not a multiple of 4 bytes (e.g. 3, so an int is too large for that move).

Answer 2

Replace the string with malloc with this one:

int* midbuf = (int *) malloc(num*sizeof(int)); 

The problem is that you allocated not num elements of int but num bytes.

Answer 3

int* midbuf = (int *) malloc(num); // This is where the access violation happens.
int* refdes = (int *) destination; // A pointer to destination, except it is casted to int*
int* refsrc = (int *) source; // Same, except with source
for (int i = 0;num >= i;i++) { 
    midbuf[i] = *(refsrc + i); // Copy source to midbuf
}

You malloc only num bytes, but in the loop, you try to copy num ints. Since an int usually takes more than one byte, you're accessing out of bounds.

Answer 4

memory access violation?

You are trying to access memory you are not entitled to access. Maybe you have a null pointer or the pointer is pointing into another program or the code segment.

Answer 5

The problem is that you're mallocing num bytes into midbuf, and then copying num ints into it. Since an int is larger than a byte on most platforms, you have a problem. Change your malloc to num*sizeof(int) and you won't have that problem.

Answer 6

There are two problems to be looked at

1. Memory space

(provided that MYmemmove does a custom implementation to move ints as the question suggests)

    int* midbuf = (int *) malloc(num * sizeof(int));

malloc is byte based, and will allocate num bytes. int * is a pointer to ints. Meaning midbuf[x] will access the memory from midbuf + sizeof(int)*x. You want to allocate num ints instead (the size of an int depends on architectures, it is usually 4 or 2 bytes). Thus the malloc(num * sizeof(int)).

2. Array indexes

    for (int i = 0;num > i;i++) { 

in C (and C++) arrays are 0-based, i.e. the first index is 0. You did it correctly. But that means also that if you reserve num ints, the useable indexes will be from 0 to num-1. In your loops, i will vary from 0 to num, thanks to the condition num >= i, meaning you will access num+1 items. So num > i (or i < num) would be a better for condition.

Answer 7

#include <stdlib.h>  // did you included this?


void* MYmemmove (void* destination, const void* source, size_t num) { 

    char *Source = source, *Destination = destination;
    char *Middle = malloc( sizeof(char) * num );    // midbuf

    for (int i = 0; i < num ; i++) { 
        Middle[i] = Destination[i]; // Copy source to midbuf
    }
    for (int i = 0; i < num ; i++) { 
        Destination[i] = Middle[i]; // Copy midbuf to destination
    }

    free(Middle);                   // free memory allocated previously with malloc
    return destination;
}

Acces violation could occur because you didn't included libraries needed for malloc (standard c doesn't throw errors in your face if you forgot a function definition). You don't need to mark pointers as NULL since there's no garbage collection in C (pointers are just that - pointers. Addresses to a point in memory, not memory themselves).

think about pointers like

addres of pointer 0x1243 0x4221 Destination -> {some kind of data}

Destination = 0x1243 *Destination = whatever value is currently at addres 0x4221

You can't also index void pointers. You must first cast them to some type, so the compiler knows how much offset they need.

Destination[x] = *(Destination+x)

a char is 1 byte, so char pointer will really get moved by x bytes, but an int is 4 bytes, and an int pointer will get moved by 4*x bytes. Don't worry too much if it sounds technical, it'll be important when you'll get to something really low level ;)