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;
}
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.