cstackvalgrindmemcpymemmove

Valgrind memcpy Invalid write of size 8 (uintptr_t *)


I have an issue with memcpy and valgrind, telling me about an Invalid write of size 8. I got to the point of figuring out where the faulty code is, but I have no clue as to why it is faulty... I'm aware that there are other questions regarding that, but they don't help me really.

The following is an excerpt of the most important bits of my approach on a somewhat "universal" stack, when my regular value would be of type uintptr_t.

Here are two defines that I used below:

// default stack batch size
#define STACK_BATCH_DEFAULT 8
// size of one value in the stack
#define STACK_SIZEOF_ONE    sizeof(uintptr_t)

The structure of the stack is as follows:

typedef struct Stack
{
    size_t count;       // count of values in the stack
    size_t size;        // size of one value in bytes
    size_t alloced;     // allocated count
    uintptr_t *value;   // the values
    int batch;          // memory gets allocated in those batches
}
Stack;

I have an initialization function for the stack:

bool stack_init(Stack *stack, size_t size, int batch)
{
    if(!stack) return false;
    stack->batch = batch ? batch : STACK_BATCH_DEFAULT;
    stack->size = size;
    stack->count = 0;
    stack->value = 0;
    stack->alloced = 0;
    return true;
}

Then the stack_push function, where valgrind throws the error Invalid write of size 8:

bool stack_push(Stack *stack, uintptr_t *value)
{
    if(!stack || !value) return false;
    // calculate required amount of elements
    size_t required = stack->batch * (stack->count / stack->batch + 1);
    // allocate more memory if we need to
    if(required > stack->alloced)
    {
        uintptr_t *tmp = realloc(stack->value, required * stack->size);
        if(!tmp) return false;
        stack->value = tmp;
        stack->alloced = required;
    }
    // set the value
    if(stack->size > STACK_SIZEOF_ONE)
    {
        memcpy(stack->value + stack->size * stack->count, value, stack->size);  // <--- valgrind throws the error here
    }
    else
    {
        stack->value[stack->count] = *value;
    }
    // increment count
    stack->count++;
    return true;
}

Then in my program I'm calling the functions as follows:

Stack stack = {0};  
stack_init(&stack, sizeof(SomeStruct), 0);
/* ... */
SomeStruct push = { // this is a struct that is larger than STACK_SIZEOF_ONE
    .int_a = 0,
    .int_b = 0,
    .int_c = 0,
    .id = 0,
    .pt = pointer_to_struct,    // it is a pointer to some other struct that was allocated beforehand
};
stack_push(&stack, (uintptr_t *)&push);

And with universal I meant that I can also have a regular stack:

Stack stack = {0};
stack_init(&stack, sizeof(uintptr_t), 0);
/* ... */
uintptr_t a = 100;
stack_push(&stack, &a);

Also, I'm open to hear general tips and advices if there are any things that should/could be improved :)

Edit: Below is a runnable code.

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

// default stack batch size
#define STACK_BATCH_DEFAULT 8
// size of one value in the stack
#define STACK_SIZEOF_ONE    sizeof(uintptr_t)

#define TESTCOUNT   10
#define MAX_BUF     16

typedef struct Stack
{
    size_t count;       // count of values in the stack
    size_t size;        // size of one value in bytes
    size_t alloced;     // allocated count
    uintptr_t *value;   // the values
    int batch;          // memory gets allocated in those batches
}
Stack;

typedef struct SomeStruct
{
    size_t a;
    size_t b;
    size_t c;
    size_t id;
    char *str;
}
SomeStruct;

bool stack_init(Stack *stack, size_t size, int batch)
{
    if(!stack) return false;
    stack->batch = batch ? batch : STACK_BATCH_DEFAULT;
    stack->size = size;
    stack->count = 0;
    stack->value = 0;
    stack->alloced = 0;
    return true;
}

bool stack_push(Stack *stack, uintptr_t *value)
{
    if(!stack || !value) return false;
    // calculate required amount of elements
    size_t required = stack->batch * (stack->count / stack->batch + 1);
    // allocate more memory if we need to
    if(required > stack->alloced)
    {
        uintptr_t *tmp = realloc(stack->value, required * stack->size);
        if(!tmp) return false;
        stack->value = tmp;
        stack->alloced = required;
    }
    // set the value
    if(stack->size > STACK_SIZEOF_ONE)
    {
        memcpy(stack->value + stack->size * stack->count, value, stack->size);  // <--- valgrind throws the error here
    }
    else
    {
        stack->value[stack->count] = *value;
    }
    // increment count
    stack->count++;
    return true;
}

bool stack_pop(Stack *stack, uintptr_t *value)
{
    if(!stack) return false;
    if(!stack->count) return false;

    // decrement count of elements
    stack->count--;

    // return the value if we have an address
    if(value)
    {
        if(stack->size > STACK_SIZEOF_ONE)
        {
            memcpy(value, stack->value + stack->size * stack->count, stack->size);
        }
        else
        {
            *value = stack->value[stack->count];
        }
    }
    
    int required = stack->batch * (stack->count / stack->batch + 1);
    if(required < stack->alloced)
    {
        uintptr_t *tmp = realloc(stack->value, required * stack->size);
        if(!tmp) return false;
        stack->value = tmp;
        stack->alloced = required;
    }
    if(!stack->value) return false;

    return true;
}

int main(void)
{
    // initialize variables
    bool valid = false;
    Stack default_stack = {0};
    Stack some_stack = {0};

    // initialize stacks
    stack_init(&default_stack, sizeof(uintptr_t), 0);
    stack_init(&some_stack, sizeof(SomeStruct), 0);

    // test default case - push
    printf("Testing the default case, pushing...\n");
    for(int i = 0; i < TESTCOUNT; i++)
    {
        uintptr_t push = i;
        valid = stack_push(&default_stack, &push);
        if(!valid) return -1;
    }
    // ...now pop
    printf("Testing the default case, popping...\n");
    do
    {
        uintptr_t pop = 0;
        valid = stack_pop(&default_stack, &pop);
        if(valid) printf("%llu,", pop);
    }
    while(valid);
    printf("\n");

    // test some case - push
    printf("Testing some case, pushing...\n");
    for(int i = 0; i < TESTCOUNT; i++)
    {
        // generate the push struct
        SomeStruct push = {
            .a = i * 10,
            .b = i * 100,
            .c = i * 1000,
            .id = i,
            .str = 0,
        };
        // allocate a string
        push.str = malloc(MAX_BUF + 1);
        snprintf(push.str, MAX_BUF, "%d", i);
        // push
        valid = stack_push(&some_stack, (uintptr_t *)&push);
        if(!valid) return -1;
    }
    // ...now pop
    printf("Testing some case, popping...\n");
    do
    {
        SomeStruct pop = {0};
        valid = stack_pop(&some_stack, (uintptr_t *)&pop);
        if(valid)
        {
            printf("a=%d,b=%d,c=%d,id=%d,str=%s\n", pop.a, pop.b, pop.c, pop.id, pop.str);
            free(pop.str);
        }
    }
    while(valid);
    printf("\n");

    /* leave out free functions for this example.... */

    return 0;
}

Solution

  • After hours I figured it out :D The mistake happened because I very rarely do pointer arithmetic... In short, I was assuming that it would always calculate with a byte.

    Let's take a look at the lines containing:

    memcpy(stack->value + stack->size * stack->count, value, stack->size);
    

    ...and break it down, so it is more readable. And also, I'll even add a handy dandy comment in it:

    size_t offset = stack->size * stack->count; // offset in bytes
    void *dest = stack->value + offset;
    void *src = value;
    memcpy(dest, src, stack->size);
    

    Now the pro C-programmer should instantly spot the problem. It is with the calculation of stack->value + offset, where it should add offset in bytes but it is not, because the stack->value is of type uintptr_t * and not of type uint8_t *.

    So to fix it, I replaced it with this line:

    void *dest = (uint8_t *)stack->value + offset;
    

    And the code works.