buglwIP - A Lightweight TCP/IP stack - Bugs: bug #34846, LWIP segfaults from IPv6...

 
 

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

bug #34846: LWIP segfaults from IPv6 reassembly timer

Submitter:  Sebastian Unger <sebunger>
Submitted:  Thu 17 Nov 2011 12:40:51 AM UTC
   
 
Category:  IPv6 Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

Jump to the original submission

Tue 13 Mar 2012 04:31:45 PM UTC, comment #12: 

Lets assume it is fixed and reopen if Sebastian updates otherwise.

Kieran Mansley <kieranm>
Group Member
Fri 20 Jan 2012 04:22:28 PM UTC, comment #11: 

It's been working well for me, but we should run the original test to be sure.

Ivan Delamer <idelamer>
Group Member
Fri 20 Jan 2012 06:41:24 AM UTC, comment #10: 

What's the status on this? It's fixed, isn't it? Sebastian, any progress testing this again?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 29 Nov 2011 07:53:56 PM UTC, comment #9: 

I modified this again. We were checking for current_netif == NULL, but sometimes this was not true. For example, when someone sends a fragment and we free the oldest fragment in buffer, we were sending time-exceeded to the latter (as ip6_current_src() was a valid address, but the wrong one).

Ivan Delamer <idelamer>
Group Member
Tue 22 Nov 2011 08:44:48 PM UTC, comment #8: 

Thanks for fixing that oversight.

Ivan Delamer <idelamer>
Group Member
Tue 22 Nov 2011 08:43:31 PM UTC, comment #7: 

Ivan, there was an alignment problem in your fix (cannot assign (packed) ip6_addr_p_t* to ip6_addr_t*) which I just fixed (need to copy the packed address to an aligned address first).

Simon Goldschmidt <goldsimon>
Group administrator
Thu 17 Nov 2011 05:57:59 PM UTC, comment #6: 

I've pushed a fix for this, Sebastian can you run your test again, if successfull I will close this bug.

Cheers, Ivan.

Ivan Delamer <idelamer>
Group Member
Thu 17 Nov 2011 05:26:56 PM UTC, comment #5: 

Hey Sebastian, thanks for posting the bug.

I think we'll have to go with something like solution 1. I checked the patch and I see a small issue: the ICMP message might be triggered by ip6_reass_remove_oldest_datagram(), which is sometimes called from the receive callback if we need to make room for a new fragmented packet.

In this case, your patch overrides the valid ip6_current_src_addr() and that could create a problem.

I will fix this by matching the destination address in the fragmented packet to a netif, it should match the netif on which the packet was accepted originally. I think this approach is used in ND6 as well.

Ivan Delamer <idelamer>
Group Member
Thu 17 Nov 2011 07:01:45 AM UTC, comment #4: 

If the packets originate from a link-local address and you don't know the netif, you can't tell where to send the responses, or can you? If so, there's no problem with your suggestion.

> I'm assuming that the current IP data (such as the current
> source address) isn't used when not in a receive callback.


That assumption is true. That's why I suggested to set-and-reset it when we are not in the input call stack (if we have to use it for ICMP, that is).

Simon Goldschmidt <goldsimon>
Group administrator
Thu 17 Nov 2011 06:53:53 AM UTC, comment #3: 

What's the reasoning behind sending the ICMP out on the exact same interface  the packet came in on? My patch changes that and uses the normal routing to determine the interface the packet should be sent out on. I believe that's a better solution because it probably works better with mobile IP v6.

Please note that my change affects all  ICMP responses not just the time exceeded response from the reassembly timer.

I'm assuming that the current IP data (such as the current source address) isn't used when not in a receive callback. If that assumption doesn't actually hold, then my patch is going to do strange and bad things. One of the LWIP architecture gurus should have a good look at it.

Sebastian Unger <sebunger>
Thu 17 Nov 2011 06:28:21 AM UTC, comment #2: 


> the problem is the use of ip_current_netif() in icmp6_send_response


Plus it uses ip6_current_src_addr(), which also is only valid during input. However, while the IP6 source address can be found in the pbuf passed to icmp6_send_response (see the IPv4 version of that function), the netif is not noted anywhere.

Wouldn't solution 1 fail on link-local addresses when we have multiple netifs? I think a mix of 1 and 2 would be the best:

- remember the netif on input (add a member in struct ip6_reassdata)
- in ip6_reass_free_complete_datagram, check if the netif is still in netif_list (and if it's up) and set ip_current_netif to it (resetting it to NULL after icmp6_time_exceeded() returned)
- then, use that netif in icmp6_send_response or use the default routing if it is NULL

> BTW the version says CVS Head, but it really is the GIT HEAD.


Yeah, I think only project leaders can change that. However, it's clear to us what you mean. :-)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 17 Nov 2011 04:10:50 AM UTC, comment #1: 

BTW the version says CVS Head, but it really is the GIT HEAD.

Sebastian Unger <sebunger>
Thu 17 Nov 2011 12:40:51 AM UTC, original submission:  

Hi there,

When I run the IPv6Ready logo test suite phase 2 against LWIP, it segfaults in one of the tests. The following is a backtrace from gdb:

Program terminated with signal 11, Segmentation fault.
#0  0x080b6fde in ip6_select_source_address (netif=0x0, dest=0x8523650) at lwip/core/ipv6/ip6.c:221
221         if (ip6_addr_isvalid(netif_ip6_addr_state(netif, i)) &&
(gdb) bt
#0  0x080b6fde in ip6_select_source_address (netif=0x0, dest=0x8523650) at lwip/core/ipv6/ip6.c:221
#1  0x080b8111 in icmp6_send_response (p=0x8513728, code=1 '�01', data=0, type=3 '�03') at lwip/core/ipv6/icmp6.c:281
#2  0x080b8008 in icmp6_time_exceeded (p=0x8513728, c=ICMP6_TE_FRAG) at lwip/core/ipv6/icmp6.c:227
#3  0x080b8296 in ip6_reass_free_complete_datagram (ipr=0x850796c) at lwip/core/ipv6/ip6_frag.c:153
#4  0x080b821a in ip6_reass_tmr () at lwip/core/ipv6/ip6_frag.c:123
#5  0x080b38d5 in ip6_reass_timer (arg=0x0) at lwip/core/timers.c:249
#6  0x080b3b89 in sys_check_timeouts () at lwip/core/timers.c:404
(gdb) up
#1  0x080b8111 in icmp6_send_response (p=0x8513728, code=1 '�01', data=0, type=3 '�03') at lwip/core/ipv6/icmp6.c:281
281       reply_src = ip6_select_source_address(ip_current_netif(), ip6_current_src_addr());

As far as I can see, the problem is the use of ip_current_netif() in icmp6_send_response. It's documentation clearly states that it may only be called from a receive callback, but we are not in a receive callback.

The question is, how best to fix it. I can conceive of two basic solutions:
1. In icmp6_send_response, if ip_current_netif() returns 0, use the normal logic for finding the outgoing interface based on the destination address.
2. When we queue up a fragment for reassembly, we remember the interface it came in on and use that when sending the response.

However, I can think of a host of problems with 2, such as interfaces disappearing in the meantime etc. So I would lean towards solution 1.

Sebastian Unger <sebunger>

 

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

Attached Files
file #24388:  routeICMPMessages.diff.gz added by sebunger (805B - application/x-gzip - Ok, I have attached a patch that seems to fix the crash for me. Please review thoroughly as it may have undesired consequences I'm not aware of. In essence I'm now properly routing ICMP messages out towards their destination rather than sending them back )

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by idelamer (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by sebunger (Submitted the item)
  • -email is unavailable- added by sebunger
  •  

    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
    2012-03-13 kieranm StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2012-01-20 goldsimon StatusNone Ready For Test
    2011-11-17 sebunger Attached File- Added routeICMPMessages.diff.gz, #24388
    2011-11-17 sebunger Carbon-Copy- Added sebunger

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code