CODEWITHSUNDEEP
cmallocaccess-violation
JimDoddle
JimDoddle

Reputation: 51

Access violation after malloc

Could someone please help me understand the following behaviour:

I have a little piece of code for cloning a float image. The method Clone takes a pointer to another image and its dimensions as arguments.

Everything works as expected, but sometimes this line clone[i] = color; causes an Access Violation. The occurrence of the exception is not predictable neither periodic. Inspecting variables at crash time shows that Color color = source[i]; is always set and valid.

How is it possible that malloc returns a bad pointer?

The code:

typedef struct
{
    float r;
    float g;
    float b;
    float a;
} Color;

Color* Clone(Color* source, int width, int height)
{
    int s = width * height;

    Color *clone;
    clone = (Color *)malloc(s * sizeof(Color));

    if (clone)
    {
        for (int i = 0; i < s; i++)
        {
            Color color = source[i];

            // Sometimes app crash here: Access violation
            clone[i] = color;
        }
    }

    return clone;
}

Any help is much appreciated.

Update:

Platform: Windows 64bit

Values of variables at crash time:

width = 256
height = 256
s = 655536
i = 0

Upvotes: 2

Views: 3274

Answers (7)

nucode
nucode

Reputation: 91

Well I cannot upvote but my problem was the problem Lance Larsen described. I was compiling C code using Visual Studio and malloc, or in my case calloc, was not working. VS was throwing no errors at the point calloc was run, however no memory was being allocated to my struct and then later on in the code when it came time to use the struct an error would be thrown stating that a memory access violation occurred while attempting to write to memory.

As Lance advised, I included stdlib.h and that resolved the problem. So a big thanks to Lance.

Upvotes: 2

Lance Larsen
Lance Larsen

Reputation: 31

When you are compiling .c source in Visual Studio that doesn't include stdlib.h (where malloc defined as returning void*) and Visual Studio uses its own definition, where malloc returns int.

Visual studio prints:

warning C4013: 'malloc' undefined; assuming extern returning int

So your pointer is truncated at 4 bytes rather than 8. It seems this problem appears only when compiling in x64 mode in .c files.

So, solution is - just include stdlib.h.

Upvotes: 3

datenwolf
datenwolf

Reputation: 162164

This

int s = width * height;

is prone to an multiplication integer overflow. If that happens you're invoking undefined behavior (since the overflow behavior of signed integers is not defined); usually malloc will allocate a buffer too short.

EDIT: Such undefined overflow can also happen if either one of width or height is negative.

To avoid this you have to check for an multiplication overflow. The only reliable way to do this, is by using unsigned arithmetic (for which overflow behavior is defined).

if( 0 > width
 || 0 > height
){
    return ERROR_INVALID_VALUE;
}
size_t const sz_width  = width;
size_t const sz_height = height;
/* ((size_t)x) != x makes use of arithmetic conversion
 * rules to check for truncation by the cast  */
if( sz_width  != width
 || sz_height != height
){
    return ERROR_TRUNCATION;
}

/* now check if the multiplication overflows */
/* size_t is unsigned, so overflow is well behaved */
size_t const sz = sz_width * sz_height;
if( (sz / sz_width) != sz_height ) {
    return ERROR_OVERFLOW;
}

Upvotes: 2

GrahamS
GrahamS

Reputation: 10350

You don't say what your target platform is, but this:

int s = width * height;

will cause an overflow if width * height would produce a number greater than MAX_INT. The C standard only requires a signed int to store up to +32767.

Your target platform may use larger ints, especially if it is a desktop OS, but it is still bad practise.

Also your function signature allows for negative values to be passed as width or height, but your code doesn't handle that possibility.

Edit: in summary, use more appropriate types. width and height should probably be unsigned int. If so then s and i should be unsigned long.

Upvotes: 1

FredK
FredK

Reputation: 4084

The malloc call is probably not your problem (as @KarolyHorvath said, the size is not really very big). The most likely problem is that the incoming source is null or empty; you should check for that before trying to reference source[i].

Upvotes: 0

skyking
skyking

Reputation: 14360

I suppose that if you have range checked the width and height as being reasonable (so you won't overflow) the best course of action would be to try using valgrid. That way you will be able to see if you've had some memory error before this that might result in malloc misbehaving or if you've actually got a memory block that isn't large enough.

Upvotes: 1

6502
6502

Reputation: 114461

I can see nothing terribly wrong with this code. However malloc can indeed return garbage if the heap has been corrupted before. Actually quite often malloc is when one detects that something went wrong and you get an explicit "heap corruption" error message.

My suggestion is, if possible, to run the program under valgrind in the hope to catch the real bad guy that corrupts heap data structures... something that happens BEFORE calling this cloning function.

Upvotes: 3

Related Questions