Infinite loop in custom malloc()

Question

I'm trying to implement my own malloc() function in C, but I'm facing this problem. The first two allocated addresses are correct, but after that, it doesn't show the others addresses. I mean, it gets stuck in an infinite loop. However, if I remove the condition && ptr->size > BLK_SIZE, It seems to work. So, the question is, why is that condition breaking the code? How can this by solved by other means than removing the condition?

Here's my code...

/* Author : Singh*/

typedef struct block_header {
    unsigned int size : 29,
                 zero : 2,
                alloc : 1;
} block_header;

//macros
#define HEADER_BLK_SIZE sizeof(block_header) // 4 bytes header
#define ALIGNED_PAYLOAD_SIZE (((((size)-1)>>2)<<2)+4) //to align the payload
#define BLK_SIZE HEADER_BLK_SIZE + ALIGNED_PAYLOAD_SIZE //total size of a blk
#define HEAP_EXTEND_SIZE ((BLK_SIZE)*1024) //the total heap size

static void *base_heap = NULL; //base of the heap, starting point
static void *end_heap = NULL;
static block_header *freeblk = NULL;

void *mymalloc(size_t size) {
    size_t remainder_heap = (HEAP_EXTEND_SIZE) - (BLK_SIZE);

    // first time init the heap and allocate the first block
    if (!base_heap) {
        base_heap = sbrk(0);
        end_heap = sbrk(HEAP_EXTEND_SIZE);
        if (base_heap == (void*)-1 || end_heap == (void*)-1)
            return NULL;

        block_header *blk = (block_header*)base_heap;
        blk->size = BLK_SIZE;
        blk->zero = 2;
        blk->alloc = 1;

        freeblk = ((void*)(base_heap)) + BLK_SIZE;
        freeblk->size = remainder_heap;
        freeblk->zero = 2;
        freeblk->alloc = 0;

        return ((void*)blk) + HEADER_BLK_SIZE;
    } else
    if (size >= HEAP_EXTEND_SIZE) {
        return NULL;
    } else {
    //second time and the others
        block_header *ptr = (block_header*)base_heap;
        size_t i;
        i = 0;
        while (i < (HEAP_EXTEND_SIZE)) { //travel the heap
            if ((ptr->alloc) ==1 ) { //if it's allocate we go to the nxt block
                ptr = ((void*)ptr) + ((size_t)(ptr->size));
                i += ((size_t)(ptr->size));
            } else
            if ((ptr->alloc) == 0 && ptr->size > BLK_SIZE) { /*if it's free and
                                                              big enough */
                ptr->size = BLK_SIZE;
                ptr->zero = 2;
                ptr->alloc = 1;
                return ((void*)ptr) + (HEADER_BLK_SIZE);
            } else { //not big enough so we go to the next block
                ptr = ((void*)ptr) + ((size_t)(ptr->size));
                i += ((size_t)(ptr->size));
            }
        }
        return NULL; //if it does not wok
    }
}

//for testing my code
void main() {
    int *i =(int*)mymalloc(12);
    printf("pointeur i : %p\n", i);

    int *ii = (int*)mymalloc(16);
    printf("pointeur ii : %p\n", ii);

    int *iii = (int*)mymalloc(20);
    printf("pointeur iii : %p\n", iii);

    int *iiii = (int*)mymalloc(24);
    printf("pointeur iiii : %p\n", iiii);
}

score 1 · Answer 1 · answered Mar 14 '16 at 02:08

There is a major problem in your code:

#define ALIGNED_PAYLOAD_SIZE (((((size)-1)>>2)<<2)+4) //to align the payload
#define BLK_SIZE HEADER_BLK_SIZE + ALIGNED_PAYLOAD_SIZE //total size of a blk
#define HEAP_EXTEND_SIZE ((BLK_SIZE)*1024) //the total heap size

All these macros refer the some current size variable. size is the name of the argument to mymalloc, making all these identifiers non constant. This is probably not what you intent...

For example, the innocent looking test:

if(size >= HEAP_EXTEND_SIZE)

actually expands to

if(size >= ((HEADER_BLK_SIZE + (((((size)-1)>>2)<<2)+4))*1024))

Which is false unless unsigned arithmetic overflow occurs and evaluates to true by coincidence.

You must clean up you code before you can debug it effectively.

score 1 · Answer 2 · answered Mar 14 '16 at 03:27

In my opinion, the root cause is that you forget to handle the freeblock point after you alloc one block. The reason why you need to handle the freeblock point is that you need the freeblock to make some judge. Below is my modification:

    else if((ptr->alloc)==0 && ptr->size > BLK_SIZE){ /*if it's free and
                                                        big enough */
        ptr->size = BLK_SIZE;
        ptr->zero = 2;
        ptr->alloc = 1;
        freeblk = ((void *)ptr) + (size_t)(ptr->size);
        freeblk->size = freeblk->size - ptr->size;
        freeblk->zero = 2;
        freeblk->alloc = 0;
        return ((void*)ptr) + (size_t)(ptr->size);
    }

After the test, I find it works. enter code here[zzhen201@ ~] $ ./test pointeur i : 0x2097004 pointeur ii : 0x2097024 pointeur iii : 0x209703c pointeur iiii : 0x2097058

However, I did not add more tests to test whether there are any other bugs. So maybe you need to do more work to test this.

score 0 · Answer 3 · edited Mar 15 '16 at 12:42

And if I change this part :

  else{
      block_header* ptr = (block_header*) base_heap;
      size_t i;
      i = 0;
      while(i<(HEAP_EXTEND_SIZE)){ //travel the heap
        if((ptr->alloc)==1){ //if it's allocate we go to the nxt block
          ptr = ((void*)ptr) + ((size_t)(ptr->size));
          i += ((size_t)(ptr->size));
        }
        else if((ptr->alloc)==0 && ptr->size > BLK_SIZE){ /*if it's free and
                                                            big enough */
            ptr->size = BLK_SIZE;
            ptr->zero = 2;
            ptr->alloc = 1;
            return ((void*)ptr) + (HEADER_BLK_SIZE);
        }
        else{ //not big enough so we go to the next block
          ptr = ((void*)ptr) + ((size_t)(ptr->size));
          i += ((size_t)(ptr->size));
        }
      }
      return NULL; //if it does not wok
    }

by this part (it doesn't know anymore if the free block is big enough) It "works" I think ..

else{
      block_header* ptr = (block_header*) base_heap;
      size_t i;
      i = 0;
      while(i<(HEAP_EXTEND_SIZE)){
        if((ptr->alloc)==1){
          ptr = ((void*)ptr) + ((size_t)(ptr->size));
          i += ((size_t)(ptr->size));
        }
        else{
          ptr->size = BLK_SIZE;
          ptr->zero = 2;
          ptr->alloc = 1;
          return ((void*)ptr) + (HEADER_BLK_SIZE);
        }
      }
      return NULL;
    }
    return NULL;
  }

Infinite loop in custom malloc()

3 Answers3