malloc in main() or malloc in another function: allocating memory for a struct and its members

Question

When initializing a struct in C, we can allocate memory inside the main function or within another function and return a pointer to the newly created struct. This first example shows the latter; memory is allocated in Buffer_create and a pointer is returned:

#include <stdio.h>
#include "buffer.h"
int main(int argc, char *argv[])
{
 struct Buffer *tx_buffer = Buffer_create(8);
 Buffer_destroy(tx_buffer);
 return 0;
}

And this one shows how all memory allocations can be done within the main function:

#include <stdio.h>
#include "buffer.h"
int main(int argc, char *argv[])
{ 
 uint8_t *ptr_rx_buffer = malloc(sizeof(uint8_t)*8);
 struct Buffer *rx_buffer = malloc(sizeof(struct Buffer));
 Buffer2_create(rx_buffer, ptr_rx_buffer, 8);
 Buffer2_destroy(rx_buffer);
 return 0;
}

And here are the contents of the header file buffer.h:

#ifndef _buffer_h
#define _buffer_h
#include <stdint.h>
#include <stdlib.h>
struct Buffer {
 uint8_t *buffer;
 size_t size;
};
struct Buffer *Buffer_create(size_t size);
void Buffer_destroy(struct Buffer *who);
void Buffer2_create(struct Buffer *who, uint8_t *buffer, size_t size);
void Buffer2_destroy(struct Buffer *who);
#endif

And buffer.c:

#include <stdint.h>
#include <assert.h>
#include <stdlib.h>
#include "buffer.h"
struct Buffer *Buffer_create(size_t size)
{
 struct Buffer *who = malloc(sizeof(struct Buffer));
 assert(who != NULL);
 who->buffer = malloc(sizeof(uint8_t)*size);
 who->size = size;
 return who;
}
void Buffer_destroy(struct Buffer *who)
{
 assert(who != NULL);
 free(who->buffer);
 free(who);
}
void Buffer2_create(struct Buffer *who, uint8_t *buffer, size_t size)
{
 assert(who != NULL);
 who->buffer = buffer;
 who->size = size;
}
void Buffer2_destroy(struct Buffer *who)
{
 assert(who != NULL);
 free(who->buffer);
 free(who);
}

The Result

Both approaches work and the executable files for both end up being the same size.

My Question

Will either of these approaches result in memory leaks or poor performance?

Answer 1

In C, initialization and destruction should be done at the same level of abstraction. This is important because it defines who is responsible for the memory.

There are two good ways to follow this guideline:

• Allocate and deallocate in the API's init/destroy functions (your first code example). fopen does this although it maps files rather than regular memory.

  type *t = CHECK(init_t());
  ...
  CHECK(destroy_t(t));
  

• Allocate and deallocate at the call site before/after the API calls. pthread_mutex_create does this.

  type t;
  CHECK(init_t(&t));
  ...
  CHECK(destroy_t(&t));
  

It is not acceptable to allocate in an initializer and then free outside. There are many examples of this pattern in the Windows API, and it is extremely error prone. You have to check the docs for which deallocation function needs to be called each time.

I personally prefer to allocate/deallocate outside the API. That way I can use automatic variables and the return value of the initializer can be a specific error code.

• David: Thank you for your answer. In my example, which would be the API? The main function, or the functions provided by buffer.h?
• sudo rm -rf slash: You are writing a programming interface for a buffer, so the buffer is the API. main is the application that uses the API.
• David: Thank you for further clarifying it. If I understand correctly, by freeing up the return value for an error code, you're suggesting that I could make Buffer2_create and Buffer2_destroy of type int instead of void and in the end return a 0 or some error code?
• sudo rm -rf slash: @David that's right. Retuning an error code is a simple and proven idiom in C. In general, it's best to separate your successful return value and your error output value.
• Sergiy Kolodyazhnyy: In other words, something like statvfs(argv[1],&fs_usage);, where in main code we'd declare fs_usage of type struct statvfs but statvfs would allocate memory for that - that's the proper way to do it, correct ? Function takes an argument what to allocate and returns exit status

Answer 2

Looking at the performance, the two versions should perform just about identically. The second version has one less call/return, which can save a couple of CPU cycles, but if you have it multiple places in your code the additional code bytes and cache misses can overshadow that. Either way you probably won't notice a difference.

Looking at readability and maintainability, the first version is much better. You know at a glance what it is doing (rather than looking at several lines to figure it all out), you won't forget any important steps, and error checking is much easier since most of it can be handled in one place (excepting the last check for successful creation of the buffer). Debugging can also be easier, since you can set a breakpoint on the creation or destruction functions if necessary.

Answer 3

In the first case, the caller is not given any control over allocation. This limits freedom and (therefore) performance: there is no control over the number of dynamic allocations or over which memory is used for what purpose, and there are limits on how the handle to the buffer can be stored (the returned pointer to Buffer must be kept around somehow, even if we would really just want to store the Buffer by value and avoid some unnecessary double-indirection).

In the second case, the caller does have control, but Buffer2_destroy makes a very limiting assumption about how the memory was allocated so in the end the caller still has no choice. Of course by looking into the implementation details, one could see that simply not calling Buffer2_destroy enables some freedom again, but this would probably be considered a hack. All in all this approach violates the guideline "allocate and free memory in the same module, at the same level of abstraction", and doesn't get much in return.

Practically what a user of some buffer may want to do is for example:

• Having the Buffer as a local variable but its data malloc-ed.
• Having the Buffer as a local variable and making its data refer to a local array.
• Save the Buffer into some other struct or array (by value, not a pointer to a Buffer which then points to the data).
• Using (part of) a static array as the data.
• Various other such combinations..
• Allocate both the buffer data and the instance of Buffer in the same allocation.

Which is why a common advice is, where possible, do not allocate or deallocate memory, use memory supplied by the caller. This applies especially to performance-sensitive settings, where "secret malloc" is not appreciated, and custom allocators are commonly used.

• Marc.2377: I think this answer would benefit greatly from a code example.
• user555045: @Marc.2377 what would you like to see?
• David: Thank you so much for your answer @harold. I am not sure if I follow with the issue of double-indirection: I need to pass this buffer around other functions to manipulate it, and I thought it would be best to pass it as a struct and "unpack" it inside each function that uses it, rather than passing both a pointer to the data (the array) and a pointer to the size of the array to each function that uses the buffer. I can add an example to clarify my question if you wish.
• user555045: @David passing it around as a struct does not necessarily imply passing around a pointer to it, it could be passed around by value, or more importantly, accessed without an extraneous level of indirection when buffers are stored in other data structures (but maybe that's not a concern here, it depends). The surface area for bugs would increase yes, such is the price for the flexibility and its performance benefits, unfortunately.
• user555045: @David well it depends, obviously it is not so good to copy large structs, but this struct is small. Returning by value is good (avoids allocation), storing the value itself is good (avoids indirection), and then you still have the choice to pass its address to some other function