5
\$\begingroup\$

I'm trying to simulate the malloc function in C by using mmap and having 2 linked lists to act as memory blocks. The program works by reserving a region of memory and then keeps track of the free and allocated memory address with size in the linked lists. I have tested the program with normal cases like

  • free memory > needed to be allocate
  • free memory < needed to be allocate
  • free memory == needed to be allocate

… but couldn't think of special cases. Are there cases where the program fails?

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/mman.h>

struct block {
void *addr; /*start address of memory for this block */
int size;
struct block *next;
};




void *mem;
struct block *freelist;
struct block *allocated_list;



void *smalloc(unsigned int nbytes) {
struct block *cur = freelist;

 while(cur!=NULL){

        if (cur ->size >= nbytes){
            struct block *tempBlock;
            tempBlock= malloc(sizeof(struct block));
            tempBlock->addr = cur ->addr;
            tempBlock->size = nbytes;
            tempBlock->next = NULL;


            cur->addr = cur->addr + nbytes;
            cur ->size = cur ->size - nbytes;

                if(allocated_list == NULL){
                    allocated_list = tempBlock;
                }
                else{
                    cur = allocated_list;
                    while(cur->next!=NULL){
                        cur =cur->next;
                    }
                    cur->next=tempBlock;
                }


            return (tempBlock->addr);

        }
        cur = cur->next;

}

return NULL;
}


int sfree(void *addr) {
struct block *cur = allocated_list;
struct block *tempBlock;
while(cur!=NULL){
        if (cur ->next->addr == addr){
            tempBlock=cur->next;        //disjoint the matched block from the allocated_list
            cur->next = cur->next->next;
            tempBlock->next = NULL;     // set the matched block pointing to null

            cur = freelist;
            while(cur->next!=NULL){
                cur =cur->next;
                }
            cur->next=tempBlock;

            return(0);
        }
        cur= cur->next;

}
return -1;
}


void mem_init(int size) {
mem = mmap(NULL, size,  PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0);
if(mem == MAP_FAILED) {
     perror("mmap");
     exit(1);
}
else {
    freelist = malloc(sizeof(struct block));
    freelist ->size = size;
    freelist ->next = NULL;
    freelist ->addr = mem;


    allocated_list= NULL;

}

/* NOTE: this function is incomplete */

}

void mem_clean(){
struct block *cur = freelist;
struct block *tempBlock;
while(cur!=NULL){
    tempBlock = cur->next;
    munmap(cur->addr,cur->size);
    free(cur);
    cur = tempBlock;
}
cur = allocated_list;
while(cur!=NULL){
    tempBlock = cur->next;
    munmap(cur->addr,cur->size);
    free(cur);
    cur = tempBlock;
}

}
\$\endgroup\$

2 Answers 2

6
\$\begingroup\$
  1. Not sure if it's just a result from copy-and-pasting the code here but the code formatting needs improvement.

    • Inconsistent indentation
    • Inconsistent use of spaces
    • Random empty lines

    Clearly formatted code is a lot easier to read, maintain and is less prone to contain bugs.

  2. Naming is inconsistent as well, e.g. allocated_list vs freelist vs tempBlock.

  3. There is no need to place braces around the return value like this: return(0);. It should simply be return 0;. return is not a function call.

  4. In order to append blocks to the end of each of the lists you currently iterate to find the end. It would pay off to keep a tail pointer pointing to the last element of each of the lists which avoids that.

  5. I think there is a bug when calling sfree on the last block allocated. Example:

    • First call to smalloc allocates a new struct block and assigns it to allocated_list
      • This means allocated_list->next == NULL.
    • Call to sfree
      • cur points to allocated_list
      • cur != NULL
      • But cur->next->addr is invalid because cur->next == NULL
  6. I think the cleaning up is technically broken. When initializing the structure you perform a single call to mmap to obtain a set of mapped memory pages. However on clean up you munmap every single block allocated by the user of smalloc. As per munmap man page:

    The implementation will require that addr be a multiple of the page size {PAGESIZE}.

    This can't be guaranteed because the passed in addr is not aligned at all as it is purely dependent on what was allocated by the user.

    However I suspect it "simply works" because the first call to munmap unmaps the entire space mapped in originally with mmap because that's what the first addr of freelist is. Nevertheless the implementation is incorrect and works by pure coincidence.

  7. You should check the return values of malloc and munmap as well.

  8. Multiple calls to mem_init will leak memory. While it would certainly count as user error detecting this and notifying the user is good defensive programming.

\$\endgroup\$
2
\$\begingroup\$

Some observations:

  1. In smalloc(), if you allocate a block that is exactly the size of a block in freelist, you end up with a block in freelist with size 0. Instead of having zero length blocks in freelist, it would be better to simply move the block from freelist to allocated_list. Note that this would involve removing an element from freelist, which currently is never done (and may result in freelist becoming NULL).

  2. If you do get rid of zero length blocks in freelist, then in sfree(), you need to handle the case where freelist is NULL.

  3. sfree() doesn't attempt to combine adjacent free blocks. So once you fragment your memory, you will never be able to allocate large blocks any more. For example, if you allocate the max number of 64 byte blocks and free them all, you will not be able to allocate anything larger than a 64 byte block after that. You could add logic to sfree() to search for blocks adjacent to the currently freed block and combine them. You could still get fragmentation, but once you free everything, you should be completely unfragmented.

  4. You currently add new blocks to the end of your lists. There is no reason to do that because your lists aren't in any particular order. To save time, you could simply add to the beginning of your lists.

  5. If you are really using smalloc() as a replacement to malloc(), you should probably think about alignment issues. What I mean by this is that the address returned by smalloc() can be misaligned. For example, if you allocate a 1 byte block followed by a 4 byte block, you might return the following two addresses: 0x10000 and 0x10001. If you try to use the four byte block at 0x10001 as an integer pointer, your system might handle that unaligned access badly. To fix this, you could pad each request up to a certain alignment boundary (such as 4, 8, or 16) so that each block returned will be aligned to that boundary.

  6. As ChrisWue already pointed out, sfree() has a bug in it. In addition to possibly dereferencing NULL, it also can never free the first element of allocated_list. The bugs are definitely related in that you are looking one element too far ahead.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.