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

Submitted by:  Thomas Taranowski <taranowski>
Submitted on:  Fri 13 Jul 2007 05:25:50 PM UTC  
 
Category: NoneSeverity: 4 - Important
Item Group: Crash ErrorStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: None

(Jump to the original submission 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>
Project AdministratorIn charge of this item.
Mon 16 Jul 2007 05:01:44 PM UTC, comment #18:

Ok for me to close

Frédéric Bernon <fbernon>
Project 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>
Project AdministratorIn charge of this item.
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>
Project 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>
Project 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>
Project 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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project 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>
Project 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>
Project 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>
Project AdministratorIn charge of this item.
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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project Member

 

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

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

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 16 Jul 2007 05:13:00 PM UTCgoldsimonItem GroupNone=>Crash Error
      StatusIn Progress=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
    Mon 16 Jul 2007 04:59:45 PM UTCgoldsimonStatusConfirmed=>In Progress
    Fri 13 Jul 2007 06:24:27 PM UTCtaranowskiAttached File-=>Added memp.patch, #13333
    Fri 13 Jul 2007 05:36:00 PM UTCjgrubbSeverity3 - Normal=>4 - Important
      StatusNone=>Confirmed
      Planned Release=>1.3.0
    Fri 13 Jul 2007 05:25:50 PM UTCtaranowskiAttached File-=>Added memp.patch, #13331

    Back to the top


    Powered by Savane 3.1-cleanup1