buglwIP - A Lightweight TCP/IP stack - Bugs: bug #34072, UDP Broadcast is received from...

 
 

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

bug #34072: UDP Broadcast is received from wrong UDP PCB if udp_port matches

Submitter:  Andreas Becker <abecker>
Submitted:  Fri 19 Aug 2011 07:25:35 AM UTC
   
 
Category:  UDP 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

Fri 09 Sep 2011 08:31:16 PM UTC, comment #6: 

Fixed as suggested in comment #1, since that works unless we want to support overlapping IP networks (which we don't, currently).

Thanks for reporting!

Simon Goldschmidt <goldsimon>
Group administrator
Tue 30 Aug 2011 12:29:44 PM UTC, comment #5: 

I really dislike overlapping IP networks, and would not want lwIP to become less lightweight in order to support them.  If we can fix this in a lightweight way that copes with non-overlapping IP networks I think that is sufficient.

Kieran Mansley <kieranm>
Group Member
Mon 22 Aug 2011 07:10:07 AM UTC, comment #4: 

Hi Ivan,

i don't expect that the netcmp solution would lead to correct results, if we are using overlapping networks on different netifs (-> Comment #2)

I would prefer this solution ( if (broadcast))

(
  (ip_addr_isany(&pcb->local_ip) || /* don't care IP*/
  (ip_addr_cmp(&(pcb->local_ip), &(inp->ip_addr)) /* IF Local IP == PCB Local IP*/
)

Andreas Becker <abecker>
Fri 19 Aug 2011 04:08:46 PM UTC, comment #3: 

I think the netcmp would yield the same result, as this is used to accept a packet on a netif at IP layer. At the moment they both seem like OK solutions.

One thing to keep in mind, the PCB local IP could be IP_ADDR_ANY, so we need to consider both cases (as with unicast packets).

Ivan Delamer <idelamer>
Group Member
Fri 19 Aug 2011 03:56:29 PM UTC, comment #2: 

Hi Ivan,

what do you think is the benefit in
- Comparing ip_current_dest_addr with the hopefully correct Net to the PCB?
over
- Match the PCB IP with the IP from the interface, the packet arrives from.

Example with hierarchical routing

First  Netif 192.168.0.1 / 16  PCB IP: 192.168.0.1
Second Netif 192.168.1.1 / 24  PCB IP: 192.168.1.1

When something arrives from second Netif, dest IP is 192.168.1.255.
Then you could get the same bad PCB match.

I think wie could expect that the user uses the same IP at the PCB and the local IP.




Andreas Becker <abecker>
Fri 19 Aug 2011 03:25:21 PM UTC, comment #1: 

I suggest adding an ip_addr_netcmp for broadcast packets in udp.c

(line 194)

#if IP_SOF_BROADCAST_RECV
            (broadcast && (pcb->so_options & SOF_BROADCAST) &&
             ip_addr_netcmp(ipX_2_ip(&pcb->local_ip), ip_current_dest_addr(), &inp->netmask)))))) {
#else  /* IP_SOF_BROADCAST_RECV */
            (broadcast &&
             ip_addr_netcmp(ipX_2_ip(&pcb->local_ip), ip_current_dest_addr(), &inp->netmask)))))) {
#endif /* IP_SOF_BROADCAST_RECV */

Ivan Delamer <idelamer>
Group Member
Fri 19 Aug 2011 07:25:35 AM UTC, original submission:  

Hi folks,

i have an issue with udp_input() IPV4 at the PCB and IP Matching.

I use two netifs with two UDP PCBs. If I receive a broadcast packet on one Interface, I want to send this to the other Interface, same UDP Ports.
But at the moment I'll receive the packet with  the wrong UDP-pcb.  Every time it receives  arrives with the same UDP PCB.

For example
Netif1: ip_addr: 192.168.0.1/24
PCB 1:  local_ip 192.168.0.1 : 1000
Netif2: ip_addr: 192.168.1.1/24
PCB 2:  local_ip 192.168.1.1 : 1000

We receive a Packet for 192.168.1.255:

udp_input(.)
{
      broadcast = ip_addr_isbroadcast(ip_current_dest_addr(), inp);

> Yes it is Broadcast on this Interface.


      for (pcb = udp_pcbs; pcb != NULL; pcb = pcb->next) {
            local_match = 0;
...
  
      /* compare PCB local addr+port to UDP destination addr+port */
      if ((pcb->local_port == dest) &&
??

> PORT match


... /* IPV 6*/

           ((
            ((!broadcast && ipX_addr_isany(0, &pcb->local_ip)) ||
            ip_addr_cmp(ipX_2_ip(&pcb->local_ip), ip_current_dest_addr()) || #if LWIP_IGMP
            ip_addr_ismulticast(ip_current_dest_addr()) || #endif /* LWIP_IGMP */ #if IP_SOF_BROADCAST_RECV
            (broadcast && (pcb->so_options & SOF_BROADCAST)))))) { #else  /* IP_SOF_BROADCAST_RECV */
            (broadcast))))) {

> YES it is Broadcast but will it be PCB1 or PCB2 ?? --> It could be the wrong PCB, depending hwo ist first on the linked list.


#endif /* IP_SOF_BROADCAST_RECV */
        local_match = 1;
...
  }

So I suggest, in case of Broadcast, we should match the netif->ip_addr with the pcb->local_ip:

      ( /* In Broadcast*/
#if IP_SOF_BROADCAST_RECV
            (broadcast && (pcb->so_options & SOF_BROADCAST)) #else  /* IP_SOF_BROADCAST_RECV */
            (broadcast)
#endif /* IP_SOF_BROADCAST_RECV */
             && ( ip_addr_isany(&pcb->local_ip) || /* don't care IP*/
  ip_addr_cmp(&(pcb->local_ip), &(inp->ip_addr)) /* IF Local IP == PCB Local IP*/

/* In Broadcast*/)

What do You think?

Andreas Becker <abecker>

 

(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 goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by idelamer (Posted a comment)
  • -email is unavailable- added by abecker (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
    2011-09-09 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
        Planned Release 1.4.1

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code