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

Submitted by:  Adam <anewman>
Submitted on:  Mon 23 Mar 2015 03:54:08 PM UTC  
 
Category:  UDP Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <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>
Project AdministratorIn charge of this item.
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>
Project 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>
Project AdministratorIn charge of this item.
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>

 

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
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    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.5