How to free() a malloc()'d structured correctly?

Question

Hi!

i have a structure malloc()'d, and after using them, i want to free() it, but my program freezes out here. Can anyone tell me, what i am doing wrong?

Here is my code:

struct data  
{  
char *filename;  
char *size;  
};   
 //primarypcs is a long type variable
struct data *primary = (struct data *)malloc( primarypcs * sizeof( struct data ) );  
memset( primary, 0, sizeof(struct data *) * primarypcs );  
...
...
...
for ( i = 0; i < primarypcs; i++ )  
{
   free( primary[i].filename );  //<----my program freezes here
   free( primary[i].size );      //<----or here
}
free( primary );  

Thanks in advance!

kampi

EDIT:

How can i correctly malloc memory for filename and size?

EDIT2:

Sorry, but i was in a hurry, and i didn't told you all the information that you need. Let me do it now :) Basicly, i want create an application, which gets the file list of two given drives/folders and then compares them. I thought (and still do), that the easiest way is, when i store the filenames and their size in a structure like mentioned above. So i have to allocate memory dynamically (i think this what they call it) for filename and size and of corse for the structure too.

Answer 1

How are the strings in the structure allocated? If they are statically assigned to constants, then don't free them that way, and all that's needed is free (primary); Freeing something which was not malloc'd will give the heap manager a heart seizure.

If the string pointers are set by malloc() or calloc(), that is the proper way.

Answer 2

You are not presenting the entire code, where a lot of things can go wrong, but one error is already obvious. The line

memset( primary, 0, sizeof(struct data *) * primarypcs );   

is not doing what you think it is doing. It is not zeroing the entire array due to type mistake in sizeof. It was most likely supposed to be

memset( primary, 0, sizeof(struct data) * primarypcs );   

Note no * under sizeof. Because of this error, most of the pointers in your array contain garbage as their initial values. If you don't set them to something meaningful in the omitted code, your calls to free will receive garbage arguments and fail.

In general, to reduce the chance of such errors it is best to avoid mentioning type names in your program, except in declarations. Since your question is tagged C++ (even though it surely looks like C), it is not possible to get rid of type casts on malloc, but otherwise I'd say that the following looks better

struct data *primary = (struct data *) malloc( primarypcs * sizeof *primary );   
memset( primary, 0, primarypcs * sizeof *primary );   

And, as a side note, if your code was intended to be C++, you could get the same result in a much more elegant, compact and portable way

data *primary = new data[primarypcs]();

Of course in this case you'll have to deallocate the memory using the appropriate C++ functionality instead of free.

Answer 3

Replacing the for loop at the bottom of your code with free (primary); should work.

Answer 4

That is because you have not explicitly allocated memory for filename and size. So trying to do free( primary[i].filename ); or free( primary[i].size ); would invoke Undefined Behavior.

Just free(primary) is enough.

EDIT:

This question has been tagged C++. So the C++ way is to use new instead of malloc for user defined types.

For differences between new and malloc have a look at this.

In C++, you just need to write

 data *primary = new data[primarypcs](); //() for value initialization

Answer 5

This fixes your problem in memset, which caused all sorts of issues for you.

memset( primary, 0, sizeof(struct data) * primarypcs );  

In short, you left uninitialized memory at the end of your 'primary' structure.

Answer 6

If you're doing this in C++, you (almost certainly) should not use something like:

data *primary = new data[primarypcs]();

Instead, you should use something like:

struct data {
    std::string filename;
    std::string size;
};

std::vector<data> primary(primarypcs);

In this case, you can typically handle the memory management much more simply: define the vector in the scope where it's needed, and when it goes out of scope, the memory will be released automatically.

Use of array new (like new x[y]) in C++ is something you're better off without. Once upon a time (15 years ago or so) it was nearly the only tool available, so its (grudging) use was almost unavoidable -- but that day is long past, and it's been at last 10 years since there was really a good reason to use it.

Since there's inevitably a comment about "except in implementing something like vector", I'll point out that, no, even when you're implementing vector you don't use array new -- you (indirectly, via an allocator) use ::operator new to allocate raw memory, placement new to create objects in that memory, and explicit dtor calls to destroy objects.

Answer 7

As others have said, there are two obviously wrong things in the snippet you've shown:

1. You don't allocate memory for filename and size members of the structs just allocated,
2. Your memset() call uses the wrong size.

Your memset() call could be simplified and corrected by:

memset(primary, 0, primarypcs * sizeof *primary);

There is another subtle problem with your code: C standard doesn't guarantee that all-bits-zero is the null pointer constant (i.e., NULL), so memset() isn't the right way to set a pointer to NULL. A portable way of doing what you want to do is:

size_t i;
for (i=0; i < primarypcs; ++i) {
    primary[i].filename = NULL;
    primary[i].size = NULL;
}

To allocate memory for filename and size, it depends upon what you want. Let's say you determine that filename needs n bytes, and size needs m. Then, your loop changes to something like this:

size_t i;
for (i=0; i < primarypcs; ++i) {
    size_t n, m;
    /* get the values of n and m */
    primary[i].filename = malloc(n * sizeof *primary[i].filename);
    primary[i].size = malloc(m * sizeof *primary[i].size);
}

You can omit the multiplication with sizeof *primary[i].filename and sizeof *primary[i].size from the above if you want: C guarantees that sizeof(char) is 1. I wrote the above for completeness and for the case when filename and size change types.

Also, note that if filename is a string of length k, then you need (k+1) bytes for it, because of the terminating 0 (so n == k+1 above).

If I were to take a guess, you want size to store the length of the corresponding filename? If that is the case, size should not be a char * but a size_t. But since I don't know how you plan on using filename and size, I am not sure.

Be sure to check for the return value of malloc(). It returns NULL for failure. I omitted the check from the above code for simplicity.

Your post has been tagged C++ too, so if you are willing to use C++, there is a C++ solution available too.

Answer 8

You're failing to memset the entire array resulting in a garbage memory pointer being freed. Use calloc instead of malloc/memset to avoid this mistake:

struct data *primary = calloc(primarypcs, sizeof(struct data));

This both allocates and clears the memory. If you want to initialize all the struct data entries too:

for (i = 0; i < primarypcs; ++i) {
    primary[i].filename = malloc(...);
    primary[i].size = malloc(...);
}

(You don't describe what filename are size are, so I leave the ... in for you to fill in).