Merge pull request #1782 from benpicco/fix_malloc

check if the requested memory is really available in _sbrk_r

cpu/cc2538/cc2538_linkerscript.ld

@@ -130,8 +130,10 @@ SECTIONS
         _estack = .;
     } > ram
 
+    /* heap section */
     . = ALIGN(4);
-    _end = . ;
+    _sheap = . ;
+    _eheap = ORIGIN(ram) + LENGTH(ram);
 
     .flashcca :
     {

cpu/cc2538/syscalls.c

@@ -39,18 +39,12 @@
 #ifdef MODULE_UART0
 #include "board_uart0.h"
 #endif
-
-#ifdef CPU_MODEL_CC2538NF11
-#define SRAM_LENGTH (16 * 1024) /**< The CC2538NF11 has 16 Kb of RAM */
-#else
-#define SRAM_LENGTH (32 * 1024) /**< All other existing models of the CC2538 have 32 Kb of RAM */
-#endif
-
 /**
  * manage the heap
  */
-extern uint32_t _end;                       /* address of last used memory cell */
+extern char _sheap;                 /* start of the heap */
-caddr_t heap_top = (caddr_t) &_end + 4;
+extern char _eheap;                 /* end of the heap */
+caddr_t heap_top = (caddr_t)&_sheap + 4;
 
 #ifndef MODULE_UART0
 /**
@@ -120,25 +114,26 @@ void _exit(int n)
  * @brief Allocate memory from the heap.
  *
  * The current heap implementation is very rudimentary, it is only able to allocate
- * memory. It does not have any means to free memory again.
+ * memory. But it does not
+ * - have any means to free memory again
  *
- * @return      a pointer to the successfully allocated memory
+ * @return [description]
- * @return      -1 on error, and errno is set to ENOMEM
  */
-caddr_t _sbrk_r(struct _reent *r, size_t incr)
+caddr_t _sbrk_r(struct _reent *r, ptrdiff_t incr)
 {
     unsigned int state = disableIRQ();
-    if ((uintptr_t)heap_top + incr > SRAM_BASE + SRAM_LENGTH) {
+    caddr_t res = heap_top;
-        restoreIRQ(state);
+
+    if (((incr > 0) && ((heap_top + incr > &_eheap) || (heap_top + incr < res))) ||
+        ((incr < 0) && ((heap_top + incr < &_sheap) || (heap_top + incr > res)))) {
         r->_errno = ENOMEM;
-        return (caddr_t)-1;
+        res = (void *) -1;
-    }
+    } else {
-    else {
-        caddr_t res = heap_top;
         heap_top += incr;
-        restoreIRQ(state);
-        return res;
     }
+
+    restoreIRQ(state);
+    return res;
 }
 
 /**

cpu/nrf51822/nrf51822qfaa_linkerscript.ld

@@ -138,6 +138,8 @@ SECTIONS
         _estack = .;
     } > ram
 
+    /* heap section */
     . = ALIGN(4);
-    _end = . ;
+    _sheap = . ;
+    _eheap = ORIGIN(ram) + LENGTH(ram);
 }

cpu/nrf51822/syscalls.c

@@ -41,8 +41,9 @@
 /**
  * manage the heap
  */
-extern uint32_t _end;                       /* address of last used memory cell */
+extern char _sheap;                 /* start of the heap */
-caddr_t heap_top = (caddr_t)&_end + 4;
+extern char _eheap;                 /* end of the heap */
+caddr_t heap_top = (caddr_t)&_sheap + 4;
 
 /**
  * @brief use mutex for waiting on incoming UART chars
@@ -111,18 +112,24 @@ void _exit(int n)
  *
  * The current heap implementation is very rudimentary, it is only able to allocate
  * memory. But it does not
- * - check if the returned address is valid (no check if the memory very exists)
  * - have any means to free memory again
  *
- * TODO: check if the requested memory is really available
- *
  * @return [description]
  */
 caddr_t _sbrk_r(struct _reent *r, ptrdiff_t incr)
 {
     unsigned int state = disableIRQ();
     caddr_t res = heap_top;
-    heap_top += incr;
+
+    if (((incr > 0) && ((heap_top + incr > &_eheap) || (heap_top + incr < res))) ||
+        ((incr < 0) && ((heap_top + incr < &_sheap) || (heap_top + incr > res)))) {
+        r->_errno = ENOMEM;
+        res = (void *) -1;
+    }
+    else {
+        heap_top += incr;
+    }
+
     restoreIRQ(state);
     return res;
 }

cpu/sam3x8e/sam3x8e_linkerscript.ld

@@ -139,6 +139,8 @@ SECTIONS
         _estack = .;
     } > ram
 
+    /* heap section */
     . = ALIGN(4);
-    _end = . ;
+    _sheap = . ;
-}
+    _eheap = ORIGIN(ram) + LENGTH(ram);
+}

cpu/sam3x8e/syscalls.c

@@ -37,10 +37,9 @@
 /**
  * manage the heap
  */
-extern uint32_t _end;                       /* address of last used memory cell */
+extern char _sheap;                 /* start of the heap */
-caddr_t heap_top = (caddr_t)&_end + 4;
+extern char _eheap;                 /* end of the heap */
-
+caddr_t heap_top = (caddr_t)&_sheap + 4;
-
 /**
  * @brief Initialize NewLib, called by __libc_init_array() from the startup script
  */
@@ -77,18 +76,24 @@ void _exit(int n)
  *
  * The current heap implementation is very rudimentary, it is only able to allocate
  * memory. But it does not
- * - check if the returned address is valid (no check if the memory very exists)
  * - have any means to free memory again
  *
- * TODO: check if the requested memory is really available
- *
  * @return [description]
  */
 caddr_t _sbrk_r(struct _reent *r, ptrdiff_t incr)
 {
     unsigned int state = disableIRQ();
     caddr_t res = heap_top;
-    heap_top += incr;
+
+    if (((incr > 0) && ((heap_top + incr > &_eheap) || (heap_top + incr < res))) ||
+        ((incr < 0) && ((heap_top + incr < &_sheap) || (heap_top + incr > res)))) {
+        r->_errno = ENOMEM;
+        res = (void *) -1;
+    }
+    else {
+        heap_top += incr;
+    }
+
     restoreIRQ(state);
     return res;
 }

cpu/stm32f0/stm32f051r8_linkerscript.ld

@@ -137,6 +137,8 @@ SECTIONS
         _estack = .;
     } > ram
 
+    /* heap section */
     . = ALIGN(4);
-    _end = . ;
+    _sheap = . ;
+    _eheap = ORIGIN(ram) + LENGTH(ram);
 }

cpu/stm32f0/syscalls.c

@@ -43,8 +43,9 @@
 /**
  * manage the heap
  */
-extern uint32_t _end;                       /* address of last used memory cell */
+extern char _sheap;                 /* start of the heap */
-caddr_t heap_top = (caddr_t)&_end + 4;
+extern char _eheap;                 /* end of the heap */
+caddr_t heap_top = (caddr_t)&_sheap + 4;
 
 #ifndef MODULE_UART0
 /**
@@ -114,18 +115,24 @@ void _exit(int n)
  *
  * The current heap implementation is very rudimentary, it is only able to allocate
  * memory. But it does not
- * - check if the returned address is valid (no check if the memory very exists)
  * - have any means to free memory again
  *
- * TODO: check if the requested memory is really available
- *
  * @return [description]
  */
 caddr_t _sbrk_r(struct _reent *r, ptrdiff_t incr)
 {
     unsigned int state = disableIRQ();
     caddr_t res = heap_top;
-    heap_top += incr;
+
+    if (((incr > 0) && ((heap_top + incr > &_eheap) || (heap_top + incr < res))) ||
+        ((incr < 0) && ((heap_top + incr < &_sheap) || (heap_top + incr > res)))) {
+        r->_errno = ENOMEM;
+        res = (void *) -1;
+    }
+    else {
+        heap_top += incr;
+    }
+
     restoreIRQ(state);
     return res;
 }

cpu/stm32f1/stm32f103cb_linkerscript.ld

@@ -137,6 +137,8 @@ SECTIONS
         _estack = .;
     } > ram
 
+    /* heap section */
     . = ALIGN(4);
-    _end = . ;
+    _sheap = . ;
+    _eheap = ORIGIN(ram) + LENGTH(ram);
 }

cpu/stm32f1/stm32f103re_linkerscript.ld

@@ -137,6 +137,8 @@ SECTIONS
         _estack = .;
     } > ram
 
+    /* heap section */
     . = ALIGN(4);
-    _end = . ;
+    _sheap = . ;
+    _eheap = ORIGIN(ram) + LENGTH(ram);
 }

cpu/stm32f1/syscalls.c

@@ -43,8 +43,9 @@
 /**
  * manage the heap
  */
-extern uint32_t _end;                       /* address of last used memory cell */
+extern char _sheap;                 /* start of the heap */
-caddr_t heap_top = (caddr_t)&_end + 4;
+extern char _eheap;                 /* end of the heap */
+caddr_t heap_top = (caddr_t)&_sheap + 4;
 
 #ifndef MODULE_UART0
 /**
@@ -114,18 +115,24 @@ void _exit(int n)
  *
  * The current heap implementation is very rudimentary, it is only able to allocate
  * memory. But it does not
- * - check if the returned address is valid (no check if the memory very exists)
  * - have any means to free memory again
  *
- * TODO: check if the requested memory is really available
- *
  * @return [description]
  */
 caddr_t _sbrk_r(struct _reent *r, ptrdiff_t incr)
 {
     unsigned int state = disableIRQ();
     caddr_t res = heap_top;
-    heap_top += incr;
+
+    if (((incr > 0) && ((heap_top + incr > &_eheap) || (heap_top + incr < res))) ||
+        ((incr < 0) && ((heap_top + incr < &_sheap) || (heap_top + incr > res)))) {
+        r->_errno = ENOMEM;
+        res = (void *) -1;
+    }
+    else {
+        heap_top += incr;
+    }
+
     restoreIRQ(state);
     return res;
 }

cpu/stm32f3/stm32f303vc_linkerscript.ld

@@ -138,6 +138,8 @@ SECTIONS
         _estack = .;
     } > ram
 
+    /* heap section */
     . = ALIGN(4);
-    _end = . ;
+    _sheap = . ;
+    _eheap = ORIGIN(ram) + LENGTH(ram);
 }

cpu/stm32f3/syscalls.c

@@ -44,8 +44,9 @@
 /**
  * @brief manage the heap
  */
-extern uint32_t _end;                       /* address of last used memory cell */
+extern char _sheap;                 /* start of the heap */
-caddr_t heap_top = (caddr_t)&_end + 4;
+extern char _eheap;                 /* end of the heap */
+caddr_t heap_top = (caddr_t)&_sheap + 4;
 
 #ifndef MODULE_UART0
 /**
@@ -114,18 +115,24 @@ void _exit(int n)
  *
  * The current heap implementation is very rudimentary, it is only able to allocate
  * memory. But it does not
- * - check if the returned address is valid (no check if the memory very exists)
  * - have any means to free memory again
  *
- * TODO: check if the requested memory is really available
- *
  * @return [description]
  */
 caddr_t _sbrk_r(struct _reent *r, ptrdiff_t incr)
 {
     unsigned int state = disableIRQ();
     caddr_t res = heap_top;
-    heap_top += incr;
+
+    if (((incr > 0) && ((heap_top + incr > &_eheap) || (heap_top + incr < res))) ||
+        ((incr < 0) && ((heap_top + incr < &_sheap) || (heap_top + incr > res)))) {
+        r->_errno = ENOMEM;
+        res = (void *) -1;
+    }
+    else {
+        heap_top += incr;
+    }
+
     restoreIRQ(state);
     return res;
 }

cpu/stm32f4/stm32f407vg_linkerscript.ld

@@ -139,6 +139,8 @@ SECTIONS
         _estack = .;
     } > ram
 
+    /* heap section */
     . = ALIGN(4);
-    _end = . ;
+    _sheap = . ;
+    _eheap = ORIGIN(ram) + LENGTH(ram);
 }

cpu/stm32f4/stm32f415rg_linkerscript.ld

@@ -139,6 +139,8 @@ SECTIONS
         _estack = .;
     } > ram
 
+    /* heap section */
     . = ALIGN(4);
-    _end = . ;
+    _sheap = . ;
+    _eheap = ORIGIN(ram) + LENGTH(ram);
 }

cpu/stm32f4/syscalls.c

@@ -43,8 +43,9 @@
 /**
  * manage the heap
  */
-extern uint32_t _end;                       /* address of last used memory cell */
+extern char _sheap;                 /* start of the heap */
-caddr_t heap_top = (caddr_t)&_end + 4;
+extern char _eheap;                 /* end of the heap */
+caddr_t heap_top = (caddr_t)&_sheap + 4;
 
 #ifndef MODULE_UART0
 /**
@@ -114,18 +115,24 @@ void _exit(int n)
  *
  * The current heap implementation is very rudimentary, it is only able to allocate
  * memory. But it does not
- * - check if the returned address is valid (no check if the memory very exists)
  * - have any means to free memory again
  *
- * TODO: check if the requested memory is really available
- *
  * @return [description]
  */
 caddr_t _sbrk_r(struct _reent *r, ptrdiff_t incr)
 {
     unsigned int state = disableIRQ();
     caddr_t res = heap_top;
-    heap_top += incr;
+
+    if (((incr > 0) && ((heap_top + incr > &_eheap) || (heap_top + incr < res))) ||
+        ((incr < 0) && ((heap_top + incr < &_sheap) || (heap_top + incr > res)))) {
+        r->_errno = ENOMEM;
+        res = (void *) -1;
+    }
+    else {
+        heap_top += incr;
+    }
+
     restoreIRQ(state);
     return res;
 }

tests/malloc/Makefile

@@ -0,0 +1,18 @@
+# name of your application
+APPLICATION = test_malloc
+
+# If no BOARD is found in the environment, use this default:
+BOARD ?= native
+
+# This has to be the absolute path to the RIOT base directory:
+RIOTBASE ?= $(CURDIR)/../..
+
+# Comment this out to disable code in RIOT that does safety checking
+# which is not needed in a production environment but helps in the
+# development process:
+CFLAGS += -DDEVELHELP
+
+# Change this to 0 show compiler invocation lines by default:
+QUIET ?= 1
+
+include $(RIOTBASE)/Makefile.include

tests/malloc/main.c

@@ -0,0 +1,82 @@
+/*
+ * Copyright (C) 2013 Benjamin Valentin <benpicco@zedat.fu-berlin.de>
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser General
+ * Public License v2.1. See the file LICENSE in the top level directory for more
+ * details.
+ */
+
+/**
+ * @ingroup tests
+ * @{
+ *
+ * @file
+ * @brief	Simple malloc/free test
+ *
+ *
+ * @author	Benjamin Valentin <benpicco@zedat.fu-berlin.de>
+ *
+ * @}
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#define CHUNK_SIZE 1024
+
+struct node {
+    struct node *next;
+    void *ptr;
+};
+
+int total = 0;
+
+void fill_memory(struct node *head)
+{
+    while (head && (head->ptr = malloc(CHUNK_SIZE))) {
+        printf("Allocated %d Bytes at 0x%p, total %d\n", CHUNK_SIZE, head->ptr, total += CHUNK_SIZE);
+        memset(head->ptr, '@', CHUNK_SIZE);
+        head = head->next = malloc(sizeof(struct node));
+        head->ptr =  0;
+        head->next = 0;
+        total += sizeof(struct node);
+    }
+}
+
+void free_memory(struct node *head)
+{
+    struct node *old_head;
+
+    while (head) {
+        if (head->ptr) {
+            printf("Free %d Bytes at 0x%p, total %d\n", CHUNK_SIZE, head->ptr, total -= CHUNK_SIZE);
+            free(head->ptr);
+        }
+
+        if (head->next) {
+            old_head = head;
+            head = head->next;
+            free(old_head);
+        }
+        else {
+            free(head);
+            head = 0;
+        }
+
+        total -= sizeof(struct node);
+    }
+}
+
+int main(void)
+{
+    while (1) {
+        struct node *head = malloc(sizeof(struct node));
+        total += sizeof(struct node);
+
+        fill_memory(head);
+        free_memory(head);
+    }
+
+    return 0;
+}