How to create () a malloc () 'd structure correctly?

Question

I have a malloc () 'd structure, and after using them I want to free (), but my program freezes here. Can someone 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 store memory correctly for file name and size?

EDIT2:

Sorry, but I was in a hurry, and I did not tell you all the information you need. Let me do it now :) Basically, I want to create an application that gets a list of files from two data disks / folders, and then compares them. I thought (and still) that the easiest way is to store the files and their size in a structure, as described above. So I have to allocate memory dynamically (I think this is what they call) for the file name and its size, as well as for the structure for this structure.

Answer 1

You can’t imagine all the code where a lot can go wrong, but one mistake is already obvious. Line

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

Don't do what you think does. It does not zero out the whole array due to a type error in sizeof . Most likely it was

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

Note * sizeof . Because of this error, most pointers in your array contain garbage as their initial values. If you don't give them something meaningful in the missing code, your calls to free will get garbage arguments and crash.

In general, to reduce the likelihood of such errors, it is best to avoid mentioning type names in your program, except in declarations. Since your question is marked with C ++ (although it certainly looks like C), it is impossible to get rid of cast types in malloc , but otherwise I would say that the following looks better

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

And, as a note, if your code were 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 will have to free the memory using the corresponding C ++ function instead of free .

Answer 2

How are lines allocated in a structure? If they are statically assigned to constants, then do not release them that way, and all you need is free (primary); Releasing something that was not malloc'd will give the coachman a heart attack.

If the string pointers are given by malloc () or calloc (), this is the correct way.

Answer 3

If you do 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, as a rule, deal with memory management more easily: define a vector in the area where it is needed, and when it goes out of scope, the memory will be automatically released.

Using a new array (for example, new x[y] ) in C ++ is something you better handle. Once upon a time (15 years ago or so) it was almost the only tool, so its (reluctant) use was almost inevitable - but this day has long passed, and 10 years have passed since there really was a good reason for using it.

Since the comment "except for the implementation of something like a vector" inevitably arises, I will indicate that no, even when you implement the vector, you are not using the new array - you (indirectly, through the allocator) use ::operator new to allocate raw memory, placing a new one to create objects in this memory and explicit dtor calls to destroy the objects.

Answer 4

As others have said, in the snippet below there are two obviously wrong things:

• You do not allocate memory for filename and size members of just allocated structures,
• Your memset() call uses the wrong size.

Your memset() call can be simplified and fixed:

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

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

 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 on what you want. Say you have determined that filename requires n bytes, and size needs m . Then your loop will change 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 above for completeness and for the case, when filename and size change types.

Note that if filename is a string of length k , for this you need (k+1) bytes due to trailing 0 (so n == k+1 above).

If I were to suggest that you want size to keep the length of the corresponding filename ? If so, size should not be char * , but a size_t . But since I do not know how you plan to use filename and size , I am not sure.

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

Your message is also marked as C ++, so if you want to use C ++, there is also a C ++ solution.

Answer 5

Replacing the for loop at the bottom of the code free (primary); must work.

Answer 6

This is because you have not explicitly allocated memory for filename and size . Therefore, when trying to make free( primary[i].filename ); or free( primary[i].size ); Undefined Behavior will be called.

Just free(primary) enough.

EDIT

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

For the differences between new and malloc check this out.

In C ++, you just need to write

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

Answer 7

This fixes your problem in memset, which caused you all kinds of problems.

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

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

Answer 8

You cannot collect the entire array, as a result of which the garbage memory pointer is freed. Use calloc instead of malloc / memset to avoid this error:

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

This selects and clears the memory. If you want to also initialize all struct data records:

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

(You do not describe what file name is the size, so I leave ... for filling).