buglwIP - 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: 

Since I'm pretty sure this is fixed, I'll close it.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 16 Jul 2007 05:01:44 PM UTC, comment #18: 

Ok for me to close

Frédéric Bernon <fbernon>
Group Member
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.

If you're OK with that, I think we can close this bug.

Simon Goldschmidt <goldsimon>
Group administrator
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>
Group Member
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>
Group Member
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>
Group Member
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:

...
  if (memp != NULL) {   
    memp_tab[type] = memp->next;   
#if MEMP_OVERFLOW_CHECK
    memp->next = NULL;
    memp->file = file;
    memp->line = line;
#endif /* MEMP_OVERFLOW_CHECK */
#if MEMP_STATS
...
#endif /* MEMP_STATS */
    LWIP_ASSERT(...);
    memp = (struct memp*)((u8_t*)memp + MEMP_SIZE);
  } else {
    LWIP_DEBUGF(...);
#if MEMP_STATS
...
#endif /* MEMP_STATS */
  }

  SYS_ARCH_UNPROTECT(old_level);

  return memp;
}

OK?

Simon Goldschmidt <goldsimon>
Group administrator
Sat 14 Jul 2007 04:49:39 PM UTC, comment #12: 

re comment #11:

Exactly, that's how I'd think it would be better.

> But it increase a little bit the footprint ("more" if SYS_LIGHTWEIGHT_PROT=1).


Does it? It's one 'if' statement less, which means less code.
We could argue here if the if statement is more code than the 'return NULL' or the 'SYS_ARCH_UNPROTECT'... For a good compiler, this probably wouldn't make a difference, but why not help the other compilers?

After all, I think the code is better to read that way.

Simon Goldschmidt <goldsimon>
Group administrator
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."

Sorry Simon. So, if we want to avoid the "extra" if, we could come to the original patch with a little change:

+++ memp.c 2007-07-11 11:41:49.105656200 -0700
@@ -355,6 +355,7 @@
#if MEMP_STATS
++lwip_stats.memp[type].err;
#endif /* MEMP_STATS */
+ SYS_ARCH_UNPROTECT(old_level);
+ return NULL;
}

SYS_ARCH_UNPROTECT(old_level);

But it increase a little bit the footprint ("more" if SYS_LIGHTWEIGHT_PROT=1). Footprint vs Speed, for this specific case, I don't have any preference...

Frédéric Bernon <fbernon>
Group Member
Sat 14 Jul 2007 03:08:05 PM UTC, comment #10: 


> return memp:?memp:NULL;


I would think a compiler would do that. I'm not near my computer now, so I cant look to see right now. I can check on Monday, but this seems like a straightforward optimization.

> should return an "invalid" address (MEMP_SIZE)


Yes, it would. If MEMP_OVERFLOW_CHECK=0, then MEMP_SIZE=0, so no error.

Jared Grubb <jgrubb>
Group Member
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 ...

 return (void*)((u8_t*)memp + MEMP_SIZE);

should return an "invalid" address (MEMP_SIZE). Or there is something I don't understand... Else, how should be the fix for you?

Frédéric Bernon <fbernon>
Group Member
Sat 14 Jul 2007 08:43:41 AM UTC, comment #8: 

I don't know about the fix:
This bug only appears if MEMP_OVERFLOW_CHECK=1. With the current patch, we always have an 'if' statement (..?..:..) at the end of memp_malloc, which is not necessary for MEMP_OVERFLOW_CHECK=0.

But then again, good compilers might optimize that away since it gets

return memp:?memp:NULL;

for MEMP_OVERFLOW_CHECK=0, which should be pretty easy to optimize, shouldn't it?

Simon Goldschmidt <goldsimon>
Group administrator
Sat 14 Jul 2007 12:08:28 AM UTC, comment #7: 

Change is commited. Added a brief comment, per what Thomas was suggesting.

How's it look Thomas? If you're happy, we'll close this bug.

Jared Grubb <jgrubb>
Group Member
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>
Group Member
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).

This may seem backwards, but it can keep you from having some weird behavior in threaded environments.

Jared Grubb <jgrubb>
Group Member
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>
Group Member
Fri 13 Jul 2007 06:41:16 PM UTC, comment #3: 

One of the comments is not correct, I think. You wrote:

> create a linked list of memp elements, marking the first element in the pool to be NULL, to allow us to detect pool exhaustion


If there are no elements for table i, then the memp_tab[i]=NULL. Otherwise, the for(j) loop runs, and memp_tab[i] points to a linked list of elements of size (MEMP_SIZE+memp_sizes[i]), where the last element in the list has its "next" set to NULL.

Otherwise, more comments are always good!

Jared Grubb <jgrubb>
Group Member
Fri 13 Jul 2007 06:24:27 PM UTC, comment #2: 

Good catch. I attached the fix, with some commentary added to memp_init.

(file #13333)

Thomas Taranowski <taranowski>
Group Member
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.

  /* Need to check for NULL in case MEMP_SIZE>0 */
  return (memp ? ((void*)((u8_t*)memp + MEMP_SIZE)) : NULL);
}

Jared Grubb <jgrubb>
Group Member
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. 


Contents of the patch - also attached
+++ memp.c 2007-07-11 11:41:49.105656200 -0700
@@ -355,6 +355,7 @@
 #if MEMP_STATS
     ++lwip_stats.memp[type].err;
 #endif /* MEMP_STATS */
+ return NULL;
   }
 
   SYS_ARCH_UNPROTECT(old_level);

Thomas Taranowski <taranowski>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #13333:  memp.patch added by taranowski (1KiB - text/x-patch)
file #13331:  memp.patch added by taranowski (282B - text/x-patch)

 

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 fbernon (Posted a comment)
  • -email is unavailable- added by jgrubb (Posted a comment)
  • -email is unavailable- added by taranowski (Submitted the item)
  •  

    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 GroupNone Crash Error
        StatusIn Progress Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2007-07-16 goldsimon StatusConfirmed In Progress
    2007-07-13 taranowski Attached File- Added memp.patch, #13333
    2007-07-13 jgrubb Severity3 - Normal 4 - Important
        StatusNone Confirmed
        Planned Release 1.3.0
    2007-07-13 taranowski Attached File- Added memp.patch, #13331

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code