How to push and pop a void pointer in C
There are a few things to fix, but for a beginner that is not bad.
- pop
You need to decrement first len
(your push does correctly post-increment). This is a stack.
void mypop(void** val) {
puts(collection->items[--collection->len]);
*val = collection->items[collection->len];
}
Arrays start at 0
, so
len = 0;
items[len++] = elem1; // len is 0 for the assignment then incremented
items[len++] = elem2; // len is 1 for the assignment then incremented
then to pop values
elem2 = items[--len]; // len is first decremented to 1
elem1 = items[--len]; // len is first decremented to 0
- str
What you want is a pointer to chars, a char *
, for str1
and str2
, since pop()
will store a pointer, not a single char.
char *str1;
mypop((void **)&str1);
puts("Popped bar");
puts(str1);
char *str2;
mypop((void **)&str2);
puts("Popped foo");
puts(str2);
puts("Done");
return 0;
That should fix the visibly corrupted display. However there are a few more things of interest
- Allocation
Your programs runs because your allocation is big, and items
being inside the struct
, its space is likely covered by the whole allocation. But that makes an assumption (quite likely, to be fair), which could lead to undefined behavior in some situations.
But to be cleaner, since you have two entities to allocate, that needs two allocations
collection = malloc( sizeof *collection );
collection->items = malloc( sizeof(collection->items[0]) * 1000 );
to be both freed later on.
In this case, the structure should be
typedef struct myarray {
int len;
void **;
} MYARRAY
Since MYARRAY
itself is pretty small, you could also declare it statically
static MYARRAY collection;
- import
#import
is deprecated, please use #include
instead.
One problem is here:
void mypush(void* state) {
DATA data = { state };
int pos = collection.len++;
collection.items[pos] = &data;
}
Note that the last line of this function stores a pointer to the local variable data
into your items
array. But as soon as the mypush()
function returns, that local variable is destroyed, which means that the pointer you stored into the array is no longer valid! (it is now a dangling pointer) Most likely your segmentation fault occurs when you later on try to read from that now-invalid pointer (which invokes undefined behavior, and in this case, a crash)
To avoid that, simply store the state
variable directly, without involving a local data
variable at all. You can cast other pointer-types to (and from) void *
as necessary (as long as you are careful to make sure that your casts match the actual type of the data the pointer points to -- with void-pointers, the compiler won't tell your if you're casting to an inappropriate type!)
There are two main issues with your modified code. The first is in the mypop
function:
void
mypop(void** val) {
puts(collection->items[collection->len]);
*val = collection->items[collection->len--];
}
When the function is entered, there are a total of collection->len
in the collection->items
array, and the index of the last one is collection->len - 1
. So collection->items[collection->len]
is reading an array member that hasn't been written to yet, and allocated memory has indeterminate values before it is written. So when you call puts
on this value, you're dereferencing an invalid pointer. This invokes undefined behavior. On your machine it prints "(null)" but on mine it crashes.
This can be fixed by decrementing len
first:
void
mypop(void** val) {
collection->len--;
puts(collection->items[collection->len]);
*val = collection->items[collection->len];
}
The second problem is in how you're saving the popped values:
char str1;
mypop((void*)&str1);
puts("Popped bar");
puts(&str1);
char str2;
mypop((void*)&str2);
puts("Popped foo");
puts(&str2);
The mypop
function is expecting a void **
, i.e. the address of a void *
, but you're passing the address of a char
. When the mypop
then assigns to *val
, it tries to write sizeof(void *)
bytes (most likely either 4 or 8 bytes) to assign the value, but str1
and str2
are only sizeof(char) == 1
byte in size. So this means *val = ...
is writing past str1
and str2
into adjacent memory that doesn't belong to it. This again invokes undefined behavior.
Since a char *
is what was stored in your stack, it should be the address of a char *
that you pass to mypop
. So make str1
and str2
pointers to char
:
char *str1;
mypop((void**)&str1);
puts("Popped bar");
puts(str1);
char *str2;
mypop((void**)&str2);
puts("Popped foo");
puts(str2);
This will get your program running properly.
Also, you haven't free'ed the memory you allocated, so be sure to free(collection)
at the end of your program.
You should also use #include
instead of #import
to include header files, as the former is standardized while the latter is an extension.
Regarding your malloc:
collection = malloc( sizeof *collection + (sizeof collection->items[0] * 1000) );
This is fine. The size of a struct with a flexible array member doesn't include the size of that member. So when space for such a struct is allocated, you need the size of the struct plus size for some number of array elements. This is exactly what you've done: allocated space for the struct with a flexible array member capable of holding 1000 elements.