buglwIP - A Lightweight TCP/IP stack - Bugs: bug #44608, connectionless UDP dst address not...

 
 

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

bug #44608: connectionless UDP dst address not being checked within udp_input

Submitter:  Adam <anewman>
Submitted:  Mon 23 Mar 2015 03:54:08 PM UTC
   
 
Category:  UDP Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.4.1

Sun 30 Aug 2015 11:04:15 AM UTC, comment #4: 

Thanks guys!

Adam <anewman>
Fri 28 Aug 2015 08:24:34 AM UTC, comment #3: 

That's correct. I've just fixed that.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 27 Aug 2015 09:01:23 PM UTC, comment #2: 

I agree OP's patch is incorrect, but I believe he spotted a bug if we are adhering to traditional BSD sockets multicast semantics.

In BSD, you can only receive multicast on a socket bound to the multicast group or INADDR_ANY as long as IP_ADD_MEMBERSHIP has been enabled.

http://stackoverflow.com/questions/10692956/what-does-it-mean-to-bind-a-multicast-udp-socket

The logic here I think is missing a single check:

/* compare PCB local addr+port to UDP destination addr+port */
if (pcb->local_port == dest) {
  if (
     (!broadcast && ip_addr_isany(&pcb->local_ip)) ||
     ip_addr_cmp(&(pcb->local_ip), &current_iphdr_dest) ||
#if LWIP_IGMP
     ip_addr_ismulticast(&current_iphdr_dest) ||
#endif /* LWIP_IGMP */
     ...

1) Binding to the multicast group is satisfied by comparing pcb->local_ip to current_iphdr_dest

2) Handling for the second case needs to include a check for INADDR_ANY.  Under #if LWIP_IGMP, we need:

ip_addr_isany(&pcb->local_ip) && ip_addr_ismulticast(&current_iphdr_dest)

Without the INADDR any check, any socket whether it's bound to another multicast group (the case of OP) or a netif, or a broadcast address, would receive it.


Joel Cunningham <jcunningham>
Group Member
Thu 27 Aug 2015 08:01:39 PM UTC, comment #1: 

Seems like I haven't fully read the OP when assigning planned release 1.5.0.

Why do you need this change? pcb->multicast_ip is only the address of the interface to send on. This is used for TX routing only. Comparing it on RX seems wrong.

Why don't you just bind your pcbs to that netif's address instead?

Simon Goldschmidt <goldsimon>
Group administrator
Mon 23 Mar 2015 03:54:08 PM UTC, original submission:  

In udp_input (udp.c) the dst address is not being checked vs. the pcb when the pcb is connectionless. Arriving UDP is only checked for matching port number.

If two Multicast UDP sockets are open using different addresses but the same port number then the frame will arrive on both.

My application requires that the UDP frame is only copied to the PCB with the matching multicast address.

May I propose an enhancement to include an option to check for a match vs. pcb.multicast_ip when current_iphdr_dest is multicast? I've implemented the following in udp.c and it works for me:

FROM LINE 239
/* compare PCB local addr+port to UDP destination addr+port */
      if (pcb->local_port == dest) {
#if MC_MATCH_DSTADDRESS
        bool mc_match_dstaddress = true;
       
        if (ip_addr_ismulticast(&current_iphdr_dest))
        {
            if (!ip_addr_cmp(&(pcb->multicast_ip), &current_iphdr_dest))
            {
                mc_match_dstaddress = false;
            }
        }

        if (mc_match_dstaddress)
        {
#endif         
        if (
           (!broadcast && ip_addr_isany(&pcb->local_ip)) ||
           ip_addr_cmp(&(pcb->local_ip), &current_iphdr_dest) ||
#if LWIP_IGMP
           ip_addr_ismulticast(&current_iphdr_dest) ||
#endif /* LWIP_IGMP */
#if IP_SOF_BROADCAST_RECV
            (broadcast && ip_get_option(pcb, SOF_BROADCAST) &&
             (ip_addr_isany(&pcb->local_ip) ||
              ip_addr_netcmp(&pcb->local_ip, ip_current_dest_addr(), &inp->netmask)))) {
#else /* IP_SOF_BROADCAST_RECV */
            (broadcast &&
             (ip_addr_isany(&pcb->local_ip) ||
              ip_addr_netcmp(&pcb->local_ip, ip_current_dest_addr(), &inp->netmask)))) {
#endif /* IP_SOF_BROADCAST_RECV */
          local_match = 1;
          if ((uncon_pcb == NULL) &&
              ((pcb->flags & UDP_FLAGS_CONNECTED) == 0)) {
            /* the first unconnected matching PCB */
            uncon_pcb = pcb;
          }
        }
#if MC_MATCH_DSTADDRESS             
        }
#endif


I also added "#define MC_MATCH_DSTADDRESS 1" to opt.h. multicast_ip is set using setsockopt with the IP_MULTICAST_IF option.

Adam <anewman>

 

(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 jcunningham (Posted a comment)
  • -email is unavailable- added by goldsimon (Updated the item)
  • -email is unavailable- added by anewman (Submitted the item)
  • -email is unavailable- added by anewman
  •  

    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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-08-28 goldsimon StatusWorks For Me Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2015-08-27 goldsimon StatusNone Works For Me
        Planned Release1.5.0
    2015-08-19 goldsimon Planned Release 1.5.0
    2015-03-23 anewman Carbon-Copy- Added anewman

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code