C Memory Overflow (v2)

Question

EDIT: Updated code with new Pastebin link but it's still stopping at the info->citizens[x]->name while loop. Added realloc to loops and tidied up the code. Any more comments would be greatly appreciated

I'm having a few problems with memory allocation overflowing

http://pastebin.com/vukRGkq9 (v2)

No matter what I try, simply not enough memory is being allocated for info->citizens and gdb is often saying that it cannot access info->citizens[x]->name.

On occasion, I'll even get KERN_INVALID_ADDRESS errors directly after printf statements for strlen (Strlen is not used in the code at the point where gdb halts due to the error, but I'm assuming printf uses strlen in some way). I think it's something to do with how the structure is being allocated memory. So I was wondering if anyone could take a look?

Answer 1

You shouldn't do malloc(sizeof(PEOPLE*)), because it allocates exactly amount of bytes for pointer (4 bytes on 32bit arch).
Seems the thing you want to do is malloc(sizeof(PEOPLE) * N) where N is the max. number of PEOPLE you want to put into that memory chunk.

Answer 2

Clearly the problem lies with:

info->citizens = malloc(sizeof(PEOPLE *));
info->citizens[0] = malloc(sizeof(PEOPLE *));
info->citizens[1] = malloc(sizeof(PEOPLE *));

Think about it logically what you are trying to do here.

Answer 3

It seems you have one too many levels of indirection. Why are you using **citizens instead of *?

Also, apart from the fact that you are allocating the space for a pointer, not the struct, there are a couple of weird things, such as the local variable info on line 31 means the initial allocation is out of scope once the block closes at line 34.

You need to think more clearly about what data is where.

Answer 4

Lots of memory allocation issues with this code. Those mentioned above plus numerous others, for example:

info->citizens[masterX]->name = malloc(sizeof(char)*strlen(dp->d_name)+1);
info->citizens[masterX]->name = dp->d_name;

You cannot copy strings in C through assignment (using =). You can write this as:

info->citizens[masterX]->name = malloc(strlen(dp->d_name)+1);
strcpy(info->citizens[masterX]->name, dp->d_name);

Or you could condense the whole allocate & copy as follows:

info->citizens[masterX]->name = strdup(dp->d_name);

Similarly at lines 143/147 (except in that case you have also allocated one byte too few in your malloc call).

Answer 5

Thanks for the help everyone! I've tidied up the code a bit and added realloc so that the size of info->citizens grows only as needed.

Any more feedback would be greatly appreciated.

Answer 6

Your structs should almost certainly not contains members such as:

time_t *modtimes;
mode_t *modes;
bool *exists;

Instead you should simply use:

time_t modtimes;
mode_t modes;
bool exists;

In that way you do not need to dynamically allocate them, or subsequently release them. The reasons are that a) they're small and b) their size is known in advance. You would use:

char *name;

for a string field because it's not small and you don't know in advance how large it is.

Elsewhere in the code, you have the folllowing:

if(top)
{
  PEOPLE *info;
  info = malloc(sizeof(PEOPLE *));
}

If top is true then this code allocates a pointer and then immediately leaks it -- the scope of the second info is limited to the if statement so you can neither use it later nor can you release it later. You would need to do something like this:

PEOPLE *process(PEOPLE *info, ...)
{
  if (top)
  {
    info = malloc(sizeof(PEOPLE));
  }

  info->name = strdup("Henry James");
  info->exists = true;
  return info;
}