patchlwIP - A Lightweight TCP/IP stack - Patches: patch #7088, Support for mem_realloc moving...

 
 

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

patch #7088: Support for mem_realloc moving memory

Submitter:  Ken Smith <kensmith>
Submitted:  Tue 09 Feb 2010 09:55:09 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Wont Do Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Planned Release:  1.4.0

Jump to the original submission

Fri 19 Feb 2010 04:27:48 PM UTC, comment #13: 

Renamed mem_realloc to mem_trim as suggested in comment #11

Simon Goldschmidt <goldsimon>
Group administrator
Thu 18 Feb 2010 09:27:37 PM UTC, comment #12: 

When an lwIP user defines MEM_LIBC_MALLOC, mem_malloc, mem_free, and mem_calloc get mapped to potentially standards conformant implementations but mem_realloc gets neutralized so clearly, I am operating outside the design space.  If there is a danger that mem_realloc assumes the pointer won't move for RAM pbufs somewhere else other than the DNS module, then I should revert my mem_realloc to the neutralized function.

I agree that lwIP is consistent with itself and my change to supply a working realloc with standards compliant semantics is the error.

The only advantage I can cite is not violating the principle of least surprise.  Developers in the wild understand the usual semantics of the standard memory allocation functions.  It is not difficult to think: if mem_free can be a standards compliant free and mem_malloc can be a standards compliant malloc and mem_calloc can be a standards compliant calloc, then perhaps mem_realloc can be a standards compliant realloc.

I can not cite any performance or correctness advantages to this customization only semantical consistency with the well known meanings for the names of the memory allocation wrapper functions.

Should you decide not to integrate this patch and change the rest of the stack to accomodate it, then may I suggest that you add a comment in mem.h near the redefinitions of the mem_* names to make it clear that mem_realloc cannot be made to map to a function which might move the pointed to memory?

Ken Smith <kensmith>
Thu 18 Feb 2010 08:30:15 PM UTC, comment #11: 

It seems like a big part of this bug is the expectation  that the lwIP mem_realloc function is like the normal realloc function, when in fact it is much more limited, and at least some of this limitation is by design. Had it been called something else, like mem_trim() this confusion would likely not have arisen.

At the moment I'm not convinced there is any advantage to changing the stack and the interface to support potentially moving the data: there is no requirement in the stack for this feature.  It seems like changing the abilities of the mem_realloc function would introduce bugs into the stack (i.e. places where it is assumed that mem_realloc doesn't move the memory) and we'd just be making extra work.

The justification that this would allow people to use a general purpose realloc function as customisation instead of the lwIP-supplied realloc is an interesting one.  Would this result in any improvement?  I'd not considered that people would want to customise this function.  Can you outline a benefit in doing so?

Kieran Mansley <kieranm>
Group Member
Thu 18 Feb 2010 08:16:03 PM UTC, comment #10: 

I consider it a mistake to rely in realloc not moving memory since realloc has been allowed to move the pointer since C89 and probably before.

If an allocation has multiple owners and one of them reallocs, that is dangerous anyway because realloc has traditionally always implied a potential free.

The only place I encountered this as I mentioned in my original problem report is in the DNS module.  The lifetime of the allocation for struct pbuf* p in dns_send is the duration of that function and no further.  There is one owner.  When pbuf_realloc moves the allocation in my implementation, the subsequent free was an error.  I have worked around this problem using the patch I have submitted to your consideration.  If lwIP is implemented in such a way that it cannot tolerate the possibility that realloc might move the pointer, then so be it.  But I would hope that those places where that might be a problem could be fixed.

Finally, please keep in mind that this patch only affects the behavior of RAM pbufs.  Many of the instances where there might be multiple pointers to allocations happen due to the linked lists of pool pbufs.  This patch will never modify those pbuf's addresses.

Ken Smith <kensmith>
Thu 18 Feb 2010 07:44:47 PM UTC, comment #9: 

The potential problem is that there may be other pointers into the region of memory that is moved.  These pointers would also need to be updated.

Kieran Mansley <kieranm>
Group Member
Thu 18 Feb 2010 06:23:49 PM UTC, comment #8: 

Simon,
The patch I provided doesn't by itself guarantee the memcpy behavior you note.  It just allows for the possibility that mem_realloc might move the pointer.  If it never does, this patch will still work and the memcpy penalty you mention will not incur.

The decision as to whether to incur that penalty is up to the implementer of mem_realloc in the case of MEM_LIBC_MALLOC where the end user has supplied a realloc function.  I realize that this is not the default behavior but it strikes me as not an unlikely customization.

This patch itself incurs no overhead other than the extra dereferencing and potentially changing the value of the supplied memory address.

Keiran,
The documentation in the code does assert somewhere that it expects mem_realloc only to shrink memory and never expand it.  Perhaps this implicitly declares that it will not move it also?

All,
I consider this patch defensive coding.  When you can enable greater safety without incurring a great cost, doesn't it make sense to do so?

Ken Smith <kensmith>
Thu 18 Feb 2010 11:00:06 AM UTC, comment #7: 

I'm happy with the current behaviour, but we should add some comments to the source and/or porting docs to explain that mem_realloc() can't move the memory.

Kieran Mansley <kieranm>
Group Member
Thu 18 Feb 2010 06:34:23 AM UTC, comment #6: 

Sorry for not being too clear. Let me first how I see this:
- we agreed that for incoming packets, it wouldn't change anything
- mem_realloc is used from pbuf_realloc only
- pbuf_realloc is currently used only for packets that are about to be sent -> if we save memory, then only for a short time, and often far less than the MTU

Now at the time pbuf_realloc is called, the pbuf is already filled with the data to be sent but is too long. Instead of merely adjusting it len (and tot_len), we call pbuf_realloc to give the unused memory back to the heap.

If mem_realloc actually moves the pbuf in memory, it will have to move the contained data, too. And this will be done using something like memcpy.

This is why I said we trade memory-saving for memcpy: We have to copy data if the new (smaller) region resides at another place in memory than the old region.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 17 Feb 2010 09:37:02 PM UTC, comment #5: 

I don't see how it trades saving memory for memcpy where we could prevent it.  It simply allows for the possibility that realloc will return a new location for the allocation in the case of RAM pbufs.  Please elaborate on what you mean.

Ken Smith <kensmith>
Wed 17 Feb 2010 09:25:11 PM UTC, comment #4: 

The discussion about this went on a bit on lwip-devel but didn't really come to an end. Does anyone have a strong need for this to get in given the fact that this trades saving some memory for memcpy where we could prevent it?

Simon Goldschmidt <goldsimon>
Group administrator
Fri 12 Feb 2010 02:54:34 PM UTC, comment #3: 

Updated the summary to better reflect this entry's contents.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 11 Feb 2010 03:09:38 PM UTC, comment #2: 

I do see problems here, for example in ip_input: at the start of the function, iphdr is set to p->payload. That patch attached here reallocs the pbuf but does not change all pointers that already point into p->payload!

There are certainly other places where this change will hurt. Also, I don't think this change is really "lightweight": it leads to "realloc" using memcpy for rx packets although this wouldn't be necessary.

I very strongly vote against this going in!! Instead, for such heap implementations, mem_realloc should do nothing, as MEM_LIBC_MALLOC does!

Simon Goldschmidt <goldsimon>
Group administrator
Thu 11 Feb 2010 12:35:22 PM UTC, comment #1: 

Note that this isn't really a bug in lwIP.  The standard definition of pbuf_realloc() and mem_realloc() guarantee not to change the memory pointed to by the pbuf, just reduce the length of the pbuf.

I can't see much harm in changing the API in the way you suggest however, so I think this should go in.

Kieran Mansley <kieranm>
Group Member
Tue 09 Feb 2010 09:55:09 PM UTC, original submission:  

I am using the raw DNS API and have #define'ed MEM_LIBC_MALLOC.  I have custom implementations of malloc and realloc.  My realloc implementation, like the standard C implementation, may change the address of the allocation.  Here is the call stack that gets into a double free situation.

dns_gethosbyname
dns_enqueue
dns_check_entry
dns_send
   p is a RAM pbuf
   p is pbuf_realloced
   for RAM pbufs, pbuf_realloc calls mem_realloc
     which my implementation replaces with a
     custom realloc
   p is freed (this is the second free since
     realloc performed a free)

The newly allocated memory has a different address but that address is not given to the caller.  The original p is freed so the caller is holding invalid memory and dns_send calls free on an already freed buffer.

Please find attached a patch which resolves this issue by changing the interface to pbuf_realloc.

Ken Smith <kensmith>

 

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

Attached Files
file #19666:  pbuf-realloc.patch added by kensmith (12KiB - 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 kieranm (Posted a comment)
  • -email is unavailable- added by kensmith (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-02-19 goldsimon StatusNone Wont Do
        Open/ClosedOpen Closed
    2010-02-12 goldsimon SummaryDNS with custom malloc/realloc contains a double free in RAM pbuf realloc Support for mem_realloc moving memory
    2010-02-11 kieranm Planned ReleaseNone 1.4.0
    2010-02-09 kensmith Attached File- Added pbuf-realloc.patch, #19666

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code