CODEWITHSUNDEEP
cmemorymallocheap-memory
bogdan tudose
bogdan tudose

Reputation: 1276

How to fix Heap Corruption after dynamically struct allocation?

I am trying to dynamically allocate a matrix as a continuous memory block

typedef struct 
{
    uint32_t m_width_u32;
    uint32_t m_height_u32;
    uint16_t *m_pixels_u16;
}IMG_t;

IMG_t* img = (IMG_t*)malloc(sizeof(IMG_t));

strcpy(lv_path_to_input_image, argv[1]); 
FILE* file = fopen(file_name, "r");

fscanf(file, "%d%d", &(img->m_height_u32), &(img->m_width_u32)

img->m_pixels_u16 = malloc( sizeof(*img->m_pixels_u16) * img->m_height_u32 * img->m_width_u32);

for (unsigned int i = 0; i < img->m_height_u32; ++i)
{
    for (unsigned int j = 0; j < img->m_width_u32; ++j)
    {
        fscanf(file, "%d", img->m_pixels_u16 + img->m_height_u32 * i + j);
    }
}
fclose(file);

if (img->m_pixels_u16 != NULL)
{
    free(img->m_pixels_u16);
}
free(img);

With this allocation I got a HEAP CORRUPTION DETECTED error, meaning that I use more memory than I allocate.

However, if I extend the allocated memory only with 2 bytes more, heap corruption error is no longer present.

img->m_pixels_u16 = malloc( sizeof(*img->m_pixels_u16) * img->m_height_u32 * img->m_width_u32 +2 );

Can you please help me to understand this behavior?

Upvotes: 1

Views: 68

Answers (2)

chux
chux

Reputation: 153417

On 32-bit int systems, wrong specifier leads to UB.

OP's code is saving to a 4-byte location when it should be 2 byte. This may have benign consequences until the last array element which writes outside the bounds and nicely triggers HEAP CORRUPTION DETECTED. A +2 allocation hides the issue.

Use matching specifiers and enabled all warnings. A good compiler, well enabled, would have warned about specifier/argument mismatch and saved time.

Also see @Ôrel about wrong index calculation

uint16_t *m_pixels_u16;
...
//fscanf(file, "%d", img->m_pixels_u16 + img->m_height_u32 * i + j);
fscanf(file, "%" SCNu16, img->m_pixels_u16 + img->m_width_u32 * i + j);
//            ^^^^^^^^^                           ^^^^^^^^^^^

Same issue with (but OP might get "lucky" here)

//fscanf(file, "%d%d", &(img->m_height_u32), &(img->m_width_u32)
fscanf(file, "%" SCNu32 "%" SCNu32, &(img->m_height_u32), &(img->m_width_u32)

Other issues

sizeof(*img->m_pixels_u16) * img->m_height_u32 * img->m_width_u32 may quietly overflow. The below offers overflow detection.

unsigned long long n = 1ULL * img->m_height_u32 * img->m_width_u32;
assert(n <= SIZE_MAX/sizeof(*img->m_pixels_u16)); 
n *= sizeof(*img->m_pixels_u16);

BTW: good use of p = malloc(sizeof *p * ...) with

img->m_pixels_u16 = malloc( sizeof(*img->m_pixels_u16) * img->m_height_u32 * img->m_width_u32);

Recommend using that here too.

// IMG_t* img = (IMG_t*)malloc(sizeof(IMG_t));
IMG_t* img = malloc(sizeof *img);

Test not needed. free(NULL) is OK.

//if (img->m_pixels_u16 != NULL)
//{
    free(img->m_pixels_u16);
//}

Yet NULL test would have been useful right after the malloc(). Note image size could legitimately be zero.

img->m_pixels_u16 = malloc(n);
if (img->m_pixels_u16 == NULL && n > 0) {
  Handle_OutOfMemory();
}

Upvotes: 2

&#212;rel
&#212;rel

Reputation: 7622

You mix up height and width: first with i = 0

j got to img->m_width_u32 - 1 so at the end of the loop you are at img->m_pixels_u16 + img->m_width_u32 - 1

Next loop with i set to one should be img->m_pixels_u16 + img->m_width_u32 + j

This should be this:

for (unsigned int i = 0; i < img->m_height_u32; ++i)
{
    for (unsigned int j = 0; j < img->m_width_u32; ++j)
    {
        fscanf(file, "%d", img->m_pixels_u16 + img->m_width_u32 * i + j);
    }
}

Upvotes: 1

Related Questions