patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9294, memp: Fix...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #9294: memp: Fix memp_overflow_check_element_underflow/overflow assertion

Submitted by:  Axel Lin <axellin>
Submitted on:  Thu 23 Mar 2017 09:45:58 AM UTC  
 
Category:  None Priority:  5 - Normal
Status:  Invalid Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Planned Release:  None

( Jump to the original submission)

Fri 24 Mar 2017 02:26:22 PM UTC, comment #7: 

In that case...

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 24 Mar 2017 01:55:06 PM UTC, comment #6: 

Hi Simon,
I re-check current code, I think current code is fine.
You can close it, sorry for the noise.

Axel Lin <axellin>
Project Member
Fri 24 Mar 2017 01:13:11 PM UTC, comment #5: 

I still don't get it: do you think this is a bug in lwIP or a bug in your test? Can I close this?

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 23 Mar 2017 03:39:50 PM UTC, comment #4: 

Hi Simon,
Thanks a lot for your review.

I try below test:
p = LWIP_ALIGNMENT_CAST(struct memp*, ((u8_t*)p + MEMP_SIZE + memp_pools[i]->size + MEMP_SANITY_REGION_AFTER_ALIGNED));
p1 = LWIP_ALIGNMENT_CAST(struct memp*, (size_t)((u8_t*)p + MEMP_SIZE + memp_pools[i]->size + MEMP_SANITY_REGION_AFTER_ALIGNED));
p2 = LWIP_ALIGNMENT_CAST(struct memp*, ((u8_t*)p + MEMP_SIZE + memp_pools[i]->size + MEMP_SANITY_REGION_AFTER_ALIGNED));
p3 = LWIP_ALIGNMENT_CAST(struct memp*, (size_t)((u8_t*)p + MEMP_SIZE + memp_pools[i]->size + MEMP_SANITY_REGION_AFTER_ALIGNED));
p4 = LWIP_ALIGNMENT_CAST(struct memp*, ((u8_t*)p + MEMP_SIZE + memp_pools[i]->size + MEMP_SANITY_REGION_AFTER_ALIGNED));
p5 = LWIP_ALIGNMENT_CAST(struct memp*, (size_t)((u8_t*)p + MEMP_SIZE + memp_pools[i]->size + MEMP_SANITY_REGION_AFTER_ALIGNED));
if (p != p1) {
        LWIP_PLATFORM_DIAG(("p=%p p1=%p p2=%p p3=%p p4=%p p5=%p\n", p, p1, p2, p3, p4, p5));

Then I reset the device 5 times and got below result.
[23:23:19:097] p=0x20002d14 p1=0x20002d5c p2=0x20002d5c p3=0x20002d5c p4=0x20002d5c p5=0x20002d5c
[23:23:21:797] p=0x20002d14 p1=0x20002d5c p2=0x20002d5c p3=0x20002d5c p4=0x20002d5c p5=0x20002d5c
[23:23:23:172] p=0x20002d14 p1=0x20002d5c p2=0x20002d5c p3=0x20002d5c p4=0x20002d5c p5=0x20002d5c
[23:23:28:094] p=0x20002d14 p1=0x20002d5c p2=0x20002d5c p3=0x20002d5c p4=0x20002d5c p5=0x20002d5c
[23:23:29:272] p=0x20002d14 p1=0x20002d5c p2=0x20002d5c p3=0x20002d5c p4=0x20002d5c p5=0x20002d5c

Above memp_overflow_check_all() call is the one called in memp_init().
I still don't figure out why this could happen.

But I found if I change the order to assign p1 first, I no longer hit this issue.
i.e. In below code p1 always equal to p.

p1 = LWIP_ALIGNMENT_CAST(struct memp*, (size_t)((u8_t*)p + MEMP_SIZE + memp_pools[i]->size + MEMP_SANITY_REGION_AFTER_ALIGNED));
p = LWIP_ALIGNMENT_CAST(struct memp*, ((u8_t*)p + MEMP_SIZE + memp_pools[i]->size + MEMP_SANITY_REGION_AFTER_ALIGNED));

Axel Lin <axellin>
Project Member
Thu 23 Mar 2017 11:35:00 AM UTC, comment #3: 

Really? Let me rephrase your test:
p = p + 72;
p1 = p + 72;
ASSERT(p1 == p);

That can't work (the 2nd line works on a changed 'p'). I don't think the added size_t cast fixes anything. I assume you do have memp overflows but I don't understand why you think it is fixed after adding the extra cast.

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 23 Mar 2017 10:50:05 AM UTC, comment #2: 

My device is stm32f412rg running with FreeRTOS.
I don't override LWIP_ALIGNMENT_CAST/LWIP_CONST_CAST.

I add below change to show you the values:

diff --git a/src/core/memp.c b/src/core/memp.c
index c715273..5e86486 100644
--- a/src/core/memp.c
+++ b/src/core/memp.c
@@ -205,6 +205,7 @@ memp_overflow_check_all(void)
 {
   u16_t i, j;
   struct memp *p;
+  struct memp *p1;
   SYS_ARCH_DECL_PROTECT(old_level);
   SYS_ARCH_PROTECT(old_level);

@@ -214,6 +215,11 @@ memp_overflow_check_all(void)
       memp_overflow_check_element_overflow(p, memp_pools[i]);
       memp_overflow_check_element_underflow(p, memp_pools[i]);
       p = LWIP_ALIGNMENT_CAST(struct memp*, ((u8_t*)p + MEMP_SIZE + memp_pools[i]->size + MEMP_SANITY_REGION_AFTER_ALIGNED));
+      p1 = LWIP_ALIGNMENT_CAST(struct memp*, (size_t)((u8_t*)p + MEMP_SIZE + memp_pools[i]->size + MEMP_SANITY_REGION_AFTER_ALIGNED));
+      if (p != p1) {
+       LWIP_PLATFORM_DIAG(("p=%p p1=%p MEMP_SIZE=%u MEMP_SANITY_REGION_AFTER_ALIGNED=%u memp_pools[i]->size=%u\n",
+                           p, p1, MEMP_SIZE, MEMP_SANITY_REGION_AFTER_ALIGNED, memp_pools[i]->size));
+      }
     }
   }
   SYS_ARCH_UNPROTECT(old_level);

p=0x20002d14 p1=0x20002d5c MEMP_SIZE=28 MEMP_SANITY_REGION_AFTER_ALIGNED=16 memp_pools[i]->size=28

Axel Lin <axellin>
Project Member
Thu 23 Mar 2017 10:27:12 AM UTC, comment #1: 

Could you please provide more info:
- Which platform are you testing this on?
- Did you override the default LWIP_ALIGNMENT_CAST/LWIP_CONST_CAST macros?
- What's your MEMP_SIZE and MEMP_SANITY_REGION_AFTER_ALIGNED
- For the p/p1 you tested, what's memp_pools[i]->size?

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 23 Mar 2017 09:45:58 AM UTC, original submission:  

My debugger keep hitting below assertion in debug build.
git bicect to commit e5f9f187adfa, fix it.

        info "(gdb)Auto-loading safe path"
memp_overflow_check_element_underflow (p=0x200067fc <memp_memory_TCPIP_MSG_INPKT_base+1320>, desc=0x8088298 <memp_TCPIP_MSG_INPKT>) at src/core/memp.c:163
163         if (m[k] != 0xcd) {
(gdb) l
158     #if MEMP_SANITY_REGION_BEFORE_ALIGNED > 0
159       u16_t k;
160       u8_t *m;
161       m = (u8_t*)p + MEMP_SIZE - MEMP_SANITY_REGION_BEFORE_ALIGNED;
162       for (k = 0; k < MEMP_SANITY_REGION_BEFORE_ALIGNED; k++) {
163         if (m[k] != 0xcd) {
164           char errstr[128] = "detected memp underflow in pool ";
165           strcat(errstr, desc->desc);
166           LWIP_ASSERT(errstr, 0);
167         }
(gdb)

I also did a small test to show the difference:
p = LWIP_ALIGNMENT_CAST(struct memp*, ((u8_t*)p + MEMP_SIZE + memp_pools[i]->size + MEMP_SANITY_REGION_AFTER_ALIGNED));
p1 = LWIP_ALIGNMENT_CAST(struct memp*, (size_t)((u8_t*)p + MEMP_SIZE + memp_pools[i]->size + MEMP_SANITY_REGION_AFTER_ALIGNED));
p1=0x20002d0c p=0x20002cc4

Fixes: e5f9f187adfa ("Continue to fix incorrect casts via size_t for some platforms")

Axel Lin <axellin>
Project Member

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by axellin (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-03-24 goldsimon StatusNone => Invalid
        Open/ClosedOpen => Closed
    2017-03-23 axellin Attached File- => Added 0001-memp-Fix-memp_overflow_check_element_underflow-overf.patch, #40092

    Back to the top


    Powered by Savane 3.5