buglwIP - A Lightweight TCP/IP stack - Bugs: bug #27105, realloc() cannot replace...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #27105: realloc() cannot replace mem_realloc()

Submitted by:  Derek Guerdon <dguerdon>
Submitted on:  Sat 25 Jul 2009 04:14:22 AM UTC  
 
Category: pbufsSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: CVS Head

(Jump to the original submission Jump to the original submission)

Tue 28 Jul 2009 05:20:31 PM UTC, comment #7:

> I think it would be best to create a small inline function that
> did nothing, and use that.


Checked that in, since it's certainly the least invasive change to be done before the release. I've created a static function, since the inline statement does not exist in C. Any good compiler should optimize that function away unless its address is used (as required in #23254).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 28 Jul 2009 02:14:02 PM UTC, comment #6:

Thanks for digging up the background Derek.

Let's not regress BUG #23254. I think it would be best to create a small inline function that did nothing, and use that.

Kieran Mansley <kieranm>
Project Administrator
Tue 28 Jul 2009 01:46:28 PM UTC, comment #5:

#define mem_realloc(x,y) (x) does work.

The code actually used this but was changed in response to issue #23254, which wanted the mem_ macros to reference function pointers rather than function-like macros. This was apparently to allow the function pointers to be passed around in messages.

If a pointer to mem_realloc needs to be passed in messages, a dummy function that does nothing but return the pointer given should be implemented. But lwIP itself doesn't do this. The only place mem_realloc is used is when it's called by pbuf_realloc.

I have to strongly disagree with the statement that not making the change is low risk. I only discovered this bug it happened to me. And our compiler's library is from Dinkumware, whose libraries are widely used.

Moving the payload pointer won't work because with RAM pbuf types, the pbuf and the payload are one. This is the whole reason that the code can't use realloc. When the memory is reallocated, the pbuf pointer moves, but pbuf_realloc does not return a pointer to the new pbuf.

I think the cleanest solution would be to have pbuf_realloc return a pointer to the new pbuf.

But for 1.3.1, I think the best idea is to roll back to the pre-#23254 definition since it works. The change was made as a convenience, but isn't required by lwIP.

Derek Guerdon <dguerdon>
Tue 28 Jul 2009 06:06:08 AM UTC, comment #4:

> I think merely resetting the payload pointer based on the
> (possibly moved) pointer returned by realloc would also fix the problem.


That would be the proper solution, of course. But it would at least require a check that the pbuf's ref-count is 1 or other references will become invalid. And yes, a change like that shouldn't go into 1.3.1.

However, just leaving the pointer as it is (by #define mem_realloc(x,y) (x)) should do no harm at all, besides consuming too much memory.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 27 Jul 2009 09:18:55 PM UTC, comment #3:

A standards-compliant realloc (pick your favorite: posix, ISO C, etc.) is permitted to move the allocated memory if the size changes, whether it shrinks or grows.

The typical realloc implementation does not move the memory if the block shrinks so I agree it's pretty low-risk for production code. There are however debug versions of the malloc/realloc/free library that always move the memory whenever they're permitted to -- just to find bugs like this.

I think merely resetting the payload pointer based on the (possibly moved) pointer returned by realloc would also fix the problem. However, I certainly wouldn't argue that this change should go into 1.3.1.

Jeff Barber <jeffbabar>
Mon 27 Jul 2009 07:36:33 PM UTC, comment #2:

I did a quick test with DNS (which uses pbuf_realloc) and it works for me. Since mem_realloc is only used in pbuf_realloc in one specific case, I don't think the risk is very high.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 27 Jul 2009 03:29:30 PM UTC, comment #1:

Can anyone test something such as

#define mem_realloc(x,y) (x)

It would be great to get this into 1.3.1 but unless it can be tested I'm not going to push it in at the last second.

Kieran Mansley <kieranm>
Project Administrator
Sat 25 Jul 2009 04:14:22 AM UTC, original submission:

In mem.h, the following line causes incorrect behavior:
#define mem_realloc realloc

LWIP's mem_realloc() always returns the same pointer it is given. But STDC's realloc() may return a pointer to a different location.

This causes problems for pbuf_realloc(), which depends on the pointer not changing. When realloc() actually allocates a new area and frees the old one, writes to freed memory occur when the pbuf is accessed.

To correct this, the mem_realloc macro from above should be modified so that mem_realloc(x,y) does nothing when MEM_LIBC_MALLOC is used.

Derek Guerdon <dguerdon>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jeffbabar (Posted a comment)
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by dguerdon (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 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 28 Jul 2009 05:20:31 PM UTCgoldsimonAssigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
    Tue 28 Jul 2009 05:20:30 PM UTCgoldsimonStatusNone=>Fixed
    Tue 28 Jul 2009 02:14:02 PM UTCkieranmPlanned Release=>1.3.1

    Back to the top


    Powered by Savane 3.1-cleanup1