From e7a9ddcf723d1d7dfd8b0c1311bef6cc7533ea38 Mon Sep 17 00:00:00 2001
From: Tian Hao <tianhao@espressif.com>
Date: Tue, 14 Nov 2017 11:52:12 +0800
Subject: [PATCH] component/heap : fix heap_region_add check bug

---
 components/heap/heap_caps_init.c             | 27 +++++++++++++++++---
 components/heap/test/test_runtime_heap_reg.c | 23 ++++++++++++++++-
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/components/heap/heap_caps_init.c b/components/heap/heap_caps_init.c
index c964c655e..a2b66bfc7 100644
--- a/components/heap/heap_caps_init.c
+++ b/components/heap/heap_caps_init.c
@@ -226,16 +226,37 @@ esp_err_t heap_caps_add_region(intptr_t start, intptr_t end)
 esp_err_t heap_caps_add_region_with_caps(const uint32_t caps[], intptr_t start, intptr_t end)
 {
     esp_err_t err = ESP_FAIL;
-    if (caps == NULL || start == 0 || end == 0 || end < start) {
+    if (caps == NULL || start == 0 || end == 0 || end <= start) {
         return ESP_ERR_INVALID_ARG;
     }
 
     //Check if region overlaps the start and/or end of an existing region. If so, the
     //region is invalid (or maybe added twice)
+    /*
+     *  assume that in on region, start must be less than end (cannot equal to) !!
+     *  Specially, the 4th scenario can be allowed. For example, allocate memory from heap,
+     *  then change the capability and call this function to create a new region for special
+     *  application.
+     *  In the following chart, 'start = start' and 'end = end' is contained in 3rd scenario.
+     *  This all equal scenario is incorrect because the same region cannot be add twice. For example,
+     *  add the .bss memory to region twice, if not do the check, it will cause exception.
+     *
+     *  the existing heap region                                  s(tart)                e(nd)
+     *                                                            |----------------------|
+     *  1.add region  [Correct]   (s1<s && e1<=s)           |-----|
+     *  2.add region  [Incorrect] (s2<=s && s<e2<=e)        |---------------|
+     *  3.add region  [Incorrect] (s3<=s && e<e3)           |-------------------------------------|
+     *  4 add region  [Correct]   (s<s4<e && s<e4<=e)                  |-------|
+     *  5.add region  [Incorrect] (s<s5<e && e<e5)                     |----------------------------|
+     *  6.add region  [Correct]   (e<=s6 && e<e6)                                        |----|
+     */
+
     heap_t *heap;
     SLIST_FOREACH(heap, &registered_heaps, next) {
-        if ( start <= heap->start &&  heap->start <=end ) return ESP_FAIL;
-        if ( start <= heap->end &&  heap->end <=end ) return ESP_FAIL;
+        if ((start <= heap->start && end > heap->start)
+                || (start < heap->end && end > heap->end)) {
+            return ESP_FAIL;
+        }
     }
 
     heap_t *p_new = malloc(sizeof(heap_t));
diff --git a/components/heap/test/test_runtime_heap_reg.c b/components/heap/test/test_runtime_heap_reg.c
index 766e5ce3b..f60b1d018 100644
--- a/components/heap/test/test_runtime_heap_reg.c
+++ b/components/heap/test/test_runtime_heap_reg.c
@@ -21,7 +21,7 @@ TEST_CASE("Allocate new heap at runtime", "[heap][ignore]")
     uint32_t after_free = esp_get_free_heap_size();
     printf("Before %u after %u\n", before_free, after_free);
     /* allow for some 'heap overhead' from accounting structures */
-    TEST_ASSERT(after_free > before_free + BUF_SZ - HEAP_OVERHEAD_MAX);
+    TEST_ASSERT(after_free >= before_free + BUF_SZ - HEAP_OVERHEAD_MAX);
 }
 
 /* NOTE: This is not a well-formed unit test, it leaks memory and
@@ -45,3 +45,24 @@ TEST_CASE("Allocate new heap with new capability", "[heap][ignore]")
     TEST_ASSERT_NOT_NULL( heap_caps_malloc(ALLOC_SZ, MALLOC_CAP_INVENTED) );
 }
 
+/* NOTE: This is not a well-formed unit test.
+ * If run twice without a reset, it will failed.
+ */
+
+TEST_CASE("Add .bss memory to heap region runtime", "[heap][ignore]")
+{
+#define BUF_SZ 1000
+#define HEAP_OVERHEAD_MAX 200
+    static uint8_t s_buffer[BUF_SZ];
+
+    printf("s_buffer start %08x end %08x\n", (intptr_t)s_buffer, (intptr_t)s_buffer + BUF_SZ);
+    uint32_t before_free = esp_get_free_heap_size();
+    TEST_ESP_OK( heap_caps_add_region((intptr_t)s_buffer, (intptr_t)s_buffer + BUF_SZ) );
+    uint32_t after_free = esp_get_free_heap_size();
+    printf("Before %u after %u\n", before_free, after_free);
+    /* allow for some 'heap overhead' from accounting structures */
+    TEST_ASSERT(after_free >= before_free + BUF_SZ - HEAP_OVERHEAD_MAX);
+
+    /* Twice add must be failed */
+    TEST_ASSERT( (heap_caps_add_region((intptr_t)s_buffer, (intptr_t)s_buffer + BUF_SZ) != ESP_OK) );
+}