lwIP - A Lightweight TCP/IP stack - Bugs: bug #20478, memp_malloc returns bogus value on...
You are not allowed to post comments on this tracker with your current authentication level.
bug #20478: memp_malloc returns bogus value on malloc failure
Submitter: | Thomas Taranowski <taranowski> | ||
Submitted: | Fri 13 Jul 2007 05:25:50 PM UTC | ||
Category: | None | Severity: | 4 - Important |
Item Group: | Crash Error | Status: | Fixed |
Privacy: | Public | Assigned to: | goldsimon |
Open/Closed: | Closed | Planned Release: | None |
lwIP version: | None |
Jump to the original submission
Mon 16 Jul 2007 05:13:00 PM UTC, comment #19: |
Simon Goldschmidt <goldsimon> |
Mon 16 Jul 2007 05:01:44 PM UTC, comment #18: Ok for me to close |
Frédéric Bernon <fbernon> |
Mon 16 Jul 2007 04:59:45 PM UTC, comment #17: I already took out the next=NULL. I only let it in for MEMP_OVERFLOW_CHECK=1. That way if someone is really debugging the application, the structure is easier to read. I don't think this is so bad because MEMP_OVERFLOW_CHECK=1 wouldn't be used in realease builds anyway.
|
Simon Goldschmidt <goldsimon> |
Sun 15 Jul 2007 03:13:05 PM UTC, comment #16: I would say you could take the next=NULL out. The user never uses it, and memp_* dont need it unless it's on the list. |
Jared Grubb <jgrubb> |
Sun 15 Jul 2007 12:29:02 PM UTC, comment #15: Ok for me. Just a thing: if the "memp->next = NULL;" really useful? I think we could remove it (since, with MEMP_OVERFLOW_CHECK=0, it next will be overwritten, and the memp_free doesn't check it , but just set it)... |
Frédéric Bernon <fbernon> |
Sun 15 Jul 2007 05:52:37 AM UTC, comment #14: Yeah, put it inside the if. That is the best I think. |
Jared Grubb <jgrubb> |
Sat 14 Jul 2007 05:15:18 PM UTC, comment #13: It would be even better to have the memp += MEM_SIZE inside the body of if(memp!=NULL). That way we totally save redundant code:
|
Simon Goldschmidt <goldsimon> |
Sat 14 Jul 2007 04:49:39 PM UTC, comment #12: re comment #11:
|
Simon Goldschmidt <goldsimon> |
Sat 14 Jul 2007 03:28:39 PM UTC, comment #11: Yes, ok, that's the point I miss (explain at the beginning of the file): "We don't need to preserve the struct memp while not allocated, so we can save a little space and set MEMP_SIZE to 0."
|
Frédéric Bernon <fbernon> |
Sat 14 Jul 2007 03:08:05 PM UTC, comment #10:
|
Jared Grubb <jgrubb> |
Sat 14 Jul 2007 02:51:32 PM UTC, comment #9: I not sure to understand why with MEMP_OVERFLOW_CHECK=0, the case should not appears. If there is not memp in a pool, I think the memp[tab]==NULL, right? In this case, even with MEMP_OVERFLOW_CHECK=0, the case where memp_tab[type]==NULL at the beginning of memp_malloc should happen when we are out of memory, and in this case ...
|
Frédéric Bernon <fbernon> |
Sat 14 Jul 2007 08:43:41 AM UTC, comment #8: I don't know about the fix:
|
Simon Goldschmidt <goldsimon> |
Sat 14 Jul 2007 12:08:28 AM UTC, comment #7: Change is commited. Added a brief comment, per what Thomas was suggesting.
|
Jared Grubb <jgrubb> |
Fri 13 Jul 2007 10:51:32 PM UTC, comment #6: Ok, the last patch file is good. Check in. About comments, add them if you are sure...
|
Frédéric Bernon <fbernon> |
Fri 13 Jul 2007 06:59:35 PM UTC, comment #5: Another way of thinking about it: When creating the linked list, we are pushing new elements onto the front of list, like a stack. So, the first element processed gets next=NULL, but it is still the last one in the list (stacks are FILO).
|
Jared Grubb <jgrubb> |
Fri 13 Jul 2007 06:50:29 PM UTC, comment #4: You're right, my wording is off. Maybe I should just take a nap. The first memp->next, not memp, is set to NULL, to indicate the end of the list. The fact that the next points back in my mind, and not forward in the list throws me off. |
Thomas Taranowski <taranowski> |
Fri 13 Jul 2007 06:41:16 PM UTC, comment #3: One of the comments is not correct, I think. You wrote:
|
Jared Grubb <jgrubb> |
Fri 13 Jul 2007 06:24:27 PM UTC, comment #2: Good catch. I attached the fix, with some commentary added to memp_init. |
Thomas Taranowski <taranowski> |
Fri 13 Jul 2007 05:36:00 PM UTC, comment #1: It returns NULL as long as MEMP_SIZE==0. But the return adds MEMP_SIZE to NULL and that is a bug. We can't add the "return NULL;" where you have it, because then SYS_ARCH_UNPROTECT doesn't happen.
|
Jared Grubb <jgrubb> |
Fri 13 Jul 2007 05:25:50 PM UTC, original submission:
If malloc fails, memp_malloc needs to return a NULL value, as many places in the code rely on this function.
|
Thomas Taranowski <taranowski> |
Depends on the following items: None found
Items that depend on this one: None found
There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.
Follow 10 latest changes.
Date | Changed by | Updated Field | Previous Value | => | Replaced by |
---|---|---|---|---|---|
2007-07-16 | goldsimon | Item Group | None | Crash Error | |
Status | In Progress | Fixed | |||
Assigned to | None | goldsimon | |||
Open/Closed | Open | Closed | |||
2007-07-16 | goldsimon | Status | Confirmed | In Progress | |
2007-07-13 | taranowski | Attached File | - | Added memp.patch, #13333 | |
2007-07-13 | jgrubb | Severity | 3 - Normal | 4 - Important | |
Status | None | Confirmed | |||
Planned Release | 1.3.0 | ||||
2007-07-13 | taranowski | Attached File | - | Added memp.patch, #13331 |
Powered by Savane 3.13-caa5.
Corresponding source code
Since I'm pretty sure this is fixed, I'll close it.