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.

  1. 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
  1. 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

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