Memory alloc / generic function pointer

[ICantC]

Hi all, I have 2 quick questions, I have a pure C text game, very small, just 1 test area created so far. But already 50% of the time I get a heap corruption crash (Visual studio IDE). I see about 4 functions in the call stack, with the last function shown below. I appreciate this probably won't be enough for anyway to determine why it got corrupted, but I just want to rule out any glaringly obvious errors with the below malloc wrapper, and typical usage.

void *allocate(const size_t bytes)
{
	void *block = malloc(bytes); // heap corruption here
	if (block == ((void *)0))
	{
		out_of_memory();
		return ((void *)0);
	}
	return block;
}

option_t *option_new(const char *name) //last malloc in call stack
{
	option_t *option = allocate(sizeof(option_t));
	option->name = allocate(sizeof(strlen(name) + 1));
	strcpy(option->name, name);
	option->active = 1;
	return option;
}

a 2nd unrelated question is about function pointers, I get a build warning of:

warning C4028: formal parameter 1 different from declaration

because of:

option_t *inventory = option_new("Inventory");
inventory->func_ptr = &display_inventory;

where option_t is:

typedef struct option
{
	char *name;
	void(*func_ptr)(void);
	int active;
} option_t;

but display_inventory takes a struct param. Is this the best way to get a generic function pointer and will I have to live with the warning? The basic design idea is to have an area, which has an array of option_t pointers, the player enters a number corresponding to one of the options, and the appropriate 'action' is executed. E.g.

void parse_cmd(area_t *area, int cmd)
{
	if (cmd == ESCAPE_CMD)
		exit(0);
	if (cmd < 1 || cmd > area->numOfOptions)
	{
		m_error("INVALID COMMAND");
		return;
	}

	int option = cmd - 1;
	int isLookAround = strcmp(area->options[option]->name, "Look Around");

	if (isLookAround == 0)
		area_display_description(area);
	else
		area->options[option]->func_ptr();
}

[jonnin]

if line 3 gives trouble, check the value of bytes, it needs to be >0

[mbozzi]

Firstly, I think you meant (line 15)

allocate(strlen(name) + 1)

but wrote

allocate(sizeof(strlen(name) + 1))

instead.

Secondly,

Is this the best way to get a generic function pointer

Yes, but you must convert the function pointer into the correct type when doing anything with it. Just insert a type cast.

Finally, a question for you:
Why choose ((void*)0) when NULL would be suitable?

Is NULL defined as a literal zero on your system?
#define NULL 0
This is a problematic definition in C++, but is it problematic for C programs? If yes, how?

[AbstractionAnon]

Line 15: Why do you have sizeof() there? strlen() returns a value of type size_t which is either 4 or 8 bytes. You are effectively doing the following:

  option->name = allocate(4);  // or allocate(8); 

You are not getting the string length.

What you want is:

  option->name = allocate(strlen(name)+1);  

[ICantC]

Thanks guys! That is indeed it, I wasn't allocating enough space for the names by using sizeof incorrectly. Doesn't appear to be crashing anymore... :)

mbozzi in answer to your query, no reason not to use NULL, I was just using it there from an example

in terms of casting the function pointer, did you mean like this?

inventory->func_ptr = (void (*)(struct inventory *)) &display_inventory;

because I still get the warning

EDIT: ignore the above, I got it with

(void (*)(struct inventory *)) inventory->func_ptr = &display_inventory;

no errors :)

[mbozzi]

&display_inventory already has type void (*)(struct inventory *) so converting it into that type doesn't do anything.

I meant
inventory->func_ptr = (void (*)(void)) &display_inventory;
And then when you call it it needs to be converted back into the original type
(((void (*)(struct inventory *))(inventory->func_ptr))(&my_inventory);

no reason not to use NULL

Thanks!

[ICantC]

thanks mbozzi, that's interesting.. I guess my simple approach in 'parse_cmd()'

else
		area->options[option]->func_ptr();

is not gonna work without the approriate cast then (infact it wouldn't work even now for inventory since I'm not passing any params)

I guess I'll have to decide on universal arguments for these option functions and pass in everything I might need, so I can have it something like

else
		(((void (*)(area_t*, player_t*, inventory_t*)) area->options[option]->func_ptr(&area, &player, &inventory);

[mbozzi]

I guess I'll have to decide on universal arguments

You could pass in a generic pointer:

#include <stdio.h>

#define MAX_NAME_LENGTH 1023
#define MAX_ITEMS 31

struct inventory { char contents[MAX_NAME_LENGTH + 1][MAX_ITEMS + 1]; };

void display_inventory(void* gpinv)
{
  struct inventory *pinv = gpinv;
  if (! pinv) return;
  
  for (int i = 0; i < MAX_ITEMS; ++i)
    if (pinv->contents[i][0]) 
      puts(pinv->contents[i]);
    else break;
}

struct option { void (*pf)(void*); }; 

int main()
{
  struct inventory i = { .contents = { "banana", "apple", "peach", "" } };
  struct option    o = { .pf = &display_inventory };
  
  o.pf(&i);
}

To accommodate multiple arguments, put each argument in a structure and pass a pointer to the structure. (My C programming skills are rusty, but hopefully this gets the point across any way.)

I don't know whether the void* alternative is materially better than what you described, but I would have used a void* out of habit.

[ICantC]

thanks a lot for the advice mbozzi, I'll give that idea a try!