patchlwIP - A Lightweight TCP/IP stack - Patches: patch #6528, lwIP crash with large ICMP...

 
 

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

patch #6528: lwIP crash with large ICMP messages.

Submitter:  Bill Auerbach <billauerbach>
Submitted:  Wed 28 May 2008 07:09:52 PM UTC
   
 
Category:  None Priority:  7 - High
Status:  Done Privacy:  Public
Assigned to:  jifl Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Wed 18 Feb 2009 04:55:29 PM UTC, comment #8: 

I've just comitted the changed size of 'buf' in ip_frag.c since I don't think there's any objection. I'll have another thought about mem.c

Simon Goldschmidt <goldsimon>
Group administrator
Tue 17 Feb 2009 08:44:02 PM UTC, comment #7: 

OK, my bad, if I wanted that, I would have had to change mem_init(), too, to use the space, Jonathan, I guess your patch is fine. Can we commit it?

Apart from that, do we have a similar problem with other memory allocated in a similar way? The pattern 'allocate aligned-size and then align the resulting pointer' is used pretty often...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 17 Feb 2009 05:16:27 AM UTC, comment #6: 

I just remembered why ram_heap is MEM_SIZE_ALIGNED + (2*SIZEOF_STRUCT_MEM) + MEM_ALIGNMENT: I wanted the user to be able to allocate MEM_SIZE at minimum. And since we need one struct mem at the start and one at the end, we need 2*SIZEOF_STRUCT_MEM. Ok, maybe '+ MEM_ALIGMENT - 2' would be enough, but those 2 bytes... We can change that, but I prefer it the way it is now.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 May 2008 09:05:31 PM UTC, comment #5: 

The change to ip_frag is fine as you'd expect.  One question though and an observation:

If I ping say 2000 (or more) should I get a full response?  There is a partial response, but not enough for Ping to not just timeout.

Observation: If I ping over 1472 and less than some value.  Ping times out.  If I Ping e.g. 16000, I get "Reply from 192.168.123.120: TTL expired during reassembly."  Why do I not get that error always on a ping over MTU?

I use MEM_LIBC_MALLOC because I get from my linker end of BSS and beginning of stack and I use that for my heap.  Stack is at the end of RAM and BSS grows upwards.  If there was an init which would take heap start and heap end (or length) I'd use lwIP malloc routines.

I've studied carefully the change and agree with it.  At most you would pad to align ram_heap by MEM_ALIGNMENT-1 because the array size is a multiple of the alignment, as is the SIZEOF_STRUCT_MEM.

I'll submit an addition for mem_init with heap start and end parameters if anyone agrees it would be nice to use lwIP routines without using a statically allocated heap.

Bill

Bill Auerbach <billauerbach>
Thu 29 May 2008 12:18:00 PM UTC, comment #4: 

@Simon

From what I can see in the change history around that point, you were playing with making sure there were correct minimum sizes of things, especially vis-a-vis alignment. Perhaps it was something you thought was needed but was obsoleted by either changes you made.

At the very least, if a bit more heap space is needed, then I don't think it would be for that reason and we would need to find out the real reason as it would probably be a different size to SIZEOF_STRUCT_MEM - possibly larger even. But let's try it out until we find evidence to the contrary.


Jonathan Larmour <jifl>
Group Member
Thu 29 May 2008 12:09:19 PM UTC, comment #3: 

Bill, since you sound keen ;-), care to try out the attached patch?


(file #15764)

Jonathan Larmour <jifl>
Group Member
Thu 29 May 2008 10:49:45 AM UTC, comment #2: 

About mem.c:
I remember increasing the size of the heap a little since it was definitively too small at one point. Unfortunately that is about all I remember, hehe. So I might have to think/test again to get the size we really need there... On the other hand, since the struct mem is not that big, it's not much space wasted.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 29 May 2008 02:28:54 AM UTC, comment #1: 

I've personally been excruciatingly busy recently, but had filed his post to look at later. I've looked at it now.

The solution can probably be:

static u8_t buf[LWIP_MEM_ALIGN_SIZE(IP_FRAG_MAX_MTU+MEM_ALIGNMENT-1)];

in fact. And similarly subtract one for the cited ram_heap in mem.c. In fact, looking at ram_heap, the comment beside its definition says we need one struct mem at the end, which is true, but then adds 2*SIZEOF_STRUCT_MEM, not just SIZEOF_STRUCT_MEM. Anyone see a reason for this? Otherwise this space is just wasted. There's another struct mem at the start, but adding space after MEM_SIZE_ALIGNED won't be relevant for that.

It was added seemingly explicitly by Simon in revision 1.36 of mem.c, but the cvs log message doesn't say why.

Simon?

Jifl

Jonathan Larmour <jifl>
Group Member
Wed 28 May 2008 07:09:52 PM UTC, original submission:  

Problem: lwIP crash with large ICMP messages.

Reference:
http://lists.nongnu.org/archive/html/lwip-users/2008-04/msg00032.html

John Keil's fix for ICMP crashes with IP_FRAG defined.

Is there a reason this wasn't submitted?  I duplicated the crash and this solution does resolve it.

Bill Auerbach <billauerbach>

 

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

Attached Files
file #15764:  ip_frag2.diff added by jifl (2KiB - text/x-patch)
file #15760:  ip_frag.diff added by billauerbach (668B - application/octet-stream)

 

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 jifl (Posted a comment)
  • -email is unavailable- added by billauerbach (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-02-18 goldsimon StatusIn Progress Done
        Open/ClosedOpen Closed
    2008-05-29 jifl StatusNone In Progress
        Assigned toNone jifl
    2008-05-29 jifl Attached File- Added ip_frag2.diff, #15764
    2008-05-28 billauerbach Attached File- Added ip_frag.diff, #15760

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code