espressif / esp-lwip

Fork of lwIP (https://savannah.nongnu.org/projects/lwip/) with ESP-IDF specific patches
Other
85 stars 129 forks source link

[Bug] wrong lwip_assert in pbuf_realloc when pbuf is the last buf of ram_heap (IDFGH-10811) #58

Closed briliant-ben closed 10 months ago

briliant-ben commented 1 year ago

Wrong LWIP_ASSERT is here, https://github.com/espressif/esp-lwip/blob/4f24c9baf9101634b7c690802f424b197b3bb685/src/core/mem.c#L818C7-L818C7

when pbuf is the last buf of ram_heap, 'mem->next' would be 'MEM_SIZE_ALIGNED', so will trigger LWIP_ASSERT; The testcase would be like this:

test_case

  1. init state, only two struct mem, and first mem->nect wil be MEM_SIZE_ALIGNED;
  2. then , pbuf_alloc a large buf, which is just enough big so the ram_heap will have ONLY one buf;
  3. the we pbuf_realloc, and the LWIP_ASSERT in mem_trim will be triggered;

so, I think this LWIP_ASSERT should be removed, Please CHECK it again, Thanks.

david-cermak commented 10 months ago

Hi @briliant-ben

I concur with your findings, although I'm not too familiar with the code and most importantly this code is not used in ESP-IDF's port.

I can only comment, that the algorithm used for reallocation needs an extra space in the heap to allocate a new chunk and free the current one, then after adjusting the next and previous pointers, the new block appears on the same address. So it wouldn't probably work with your usecase, but I agree that instead of an assertion, we might better return a failure.

But since this is not used within Espressif port, and I don't feel confident enough to update it, I'd suggest posting a patch to lwip upstream. Please let me know if you need some help with designing a test case that exhibits this issue. You simply add a new test next to this:

https://github.com/espressif/esp-lwip/blob/4f24c9baf9101634b7c690802f424b197b3bb685/test/unit/core/test_mem.c#L30-L31

writing what you described above in the code:

  p1 = mem_malloc(MEM_SIZE_ALIGNED - ((SIZEOF_STRUCT_MEM + MEM_SANITY_OFFSET)));
  fail_unless(p1 != NULL);
  mem_trim(p1, 16);
david-cermak commented 10 months ago

Closing per the explanation above.