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()

Submitter:  Derek Guerdon <dguerdon>
Submitted:  Sat 25 Jul 2009 04:14:22 AM UTC
   
 
Category:  pbufs Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

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>
Group administrator
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>
Group Member
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>
Group administrator
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>
Group administrator
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>
Group Member
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>

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2009-07-28 goldsimon Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2009-07-28 goldsimon StatusNone Fixed
    2009-07-28 kieranm Planned Release 1.3.1

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code