Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
briliant-ben opened this issue Aug 5, 2023 · 2 comments

Comments

@briliant-ben
Copy link

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.

@github-actions github-actions bot changed the title [Bug] wrong lwip_assert in pbuf_realloc when pbuf is the last buf of ram_heap [Bug] wrong lwip_assert in pbuf_realloc when pbuf is the last buf of ram_heap (IDFGH-10811) Aug 5, 2023
@david-cermak
Copy link
Collaborator

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:

/** Call mem_malloc, mem_free and mem_trim and check stats */
START_TEST(test_mem_one)

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
Copy link
Collaborator

Closing per the explanation above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants