malloc implementation

Please give me some feedback to my malloc implementation. I implemented this design to understand the basic concept.

```
#include
#include
#include

#define HEAP_MEM_SIZE 150
#define MEMORY_AVAILABLE 1
#define MEMORY_USED 0

struct chunk_header {
struct chunk_header *next; // next pointer on free list
size_t size; // the size of this chunk
bool is_available; // indicates if this chunk is MEMORY_AVAILABLE or MEMORY_USED
};

typedef struct chunk_header chunk_header;
chunk_header *chunk_header_begin;
static char buffer[HEAP_MEM_SIZE];
unsigned int heap_size;

void init() {
heap_size = HEAP_MEM_SIZE;
chunk_header_begin = (chunk_header*)&buffer;
chunk_header_begin->next = NULL;
chunk_header_begin->size = heap_size;
chunk_header_begin->is_available = MEMORY_AVAILABLE;
}

void init_next_chunk(chunk_header *curr, unsigned int bytes) {
heap_size -= bytes;
curr->next = NULL;
curr->size = heap_size;
curr->is_available = MEMORY_AVAILABLE;
}

void *my_malloc(unsigned int nbytes) {
static bool init_flag = false;

if(!init_flag) {
init();
init_flag = true;
}

int alloc_size = nbytes + sizeof(chunk_header);
chunk_header *curr = chunk_header_begin;

while(curr) {
if(curr->is_available && curr->size >= alloc_size) {
curr->is_available = MEMORY_USED;
curr->size = alloc_size;
curr->next = curr + alloc_size;

init_next_chunk(curr->next, alloc_size);

// return memory region
curr = curr + sizeof(chunk_header);
return curr;
}

curr = curr->next;
}

return NULL;
}

void my_free(void *firstbyte) {
chunk_header mem = (chunk_header)firstbyte - sizeof(chunk_header);
chunk_header *curr = chunk_header_begin;

while (curr) {
if (curr == mem) {
heap_size += curr->size;

// Mark the block as being available
curr->is

Things you did well:

-
Some use of comments.

-
Use of typedef.

Things you could improve:

Syntax/Styling:

-
Never use goto.

Yes, there are some rare situations where you may find it necessary to use it. This is not one of them.

err:
        printf("could not allocate mem\n");
        return 0;

As you can see, all you do in your little block of code here is print out an unspecific error message, and then exit (using 0, which means the program was successful and is contradictory here).

You should instead abolish the goto's completely. Print out specific, useful error messages instead, and then return with a unique exit code.

if(mem2 == NULL) 
{
    fprintf(stderr, "Error allocating memory to mem2.\n");
    return -2;
}

-
Your format specifies type int when you are trying to print a string, but the argument has type size_t (basically, unsigned long). Your format also specifies type unsigned int but the argument has type void *.

printf("\n\t%d\t%d\t\t%x", curr->size, curr->is_available, mem_ptr);

Just change they type you are trying to print out, and cast mem_ptr to an unsigned int.

printf("\n\t%zuu\t%d\t\t%x", curr->size, curr->is_available, (unsigned int) mem_ptr);

-
You aren't using standard naming conventions with your typedef struct.

typedef struct chunk_header chunk_header;

-
You should be giving the typedef a specific name, different from the abstract struct name.

-
You should be capitalizing the name of the type, or adding a _t to the end (though this is reserved in POSIX)

-
typedef your structs when you declare them.

struct chunk_header {
 struct chunk_header *next; // next pointer on free list
 size_t size;               // the size of this chunk
 bool is_available;         // indicates if this chunk is MEMORY_AVAILABLE or MEMORY_USED
};
typedef struct chunk_header chunk_header;

The typedef means you no longer have to write struct all over the place, as you have used it. But you can combine these two parts to make things more simple:

typedef struct chunk_header
{
    struct chunk_header *next; // next pointer on free list
    size_t size;               // the size of this chunk
    bool is_available;         // indicates if this chunk is MEMORY_AVAILABLE or MEMORY_USED
} ChunkHeader;

-
You can simplify your NULL checks.

if(mem1 == NULL)

Since NULL is defined as (void *)0, we can treat is as a comparison to 0.

if(!mem1)

Parameters/Returns:

-
You don't accept any parameters for some of your functions, such as main(). If you do not take in any parameters, declare them as void.

int main(void)

-
You should still return from a function, even if it is declared to return void.

void my_free(void *firstbyte) {
    ...
    return;
}

Compilation:

• I can see from some warnings I received that you have not enabled all of your compiler warnings, which you should do.

StackExchange Code Review Q#43663, answer score: 14