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: UDPSeverity: 3 - Normal
Item Group: Change RequestStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned 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
  • -unavailable- added by jcunningham (Posted a comment)
  • -unavailable- added by goldsimon (Updated the item)
  • -unavailable- added by anewman (Submitted the item)
  • -unavailable- added by anewman
  •  

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

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 28 Aug 2015 08:24:34 AM UTCgoldsimonStatusWorks For Me=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
    Thu 27 Aug 2015 08:01:39 PM UTCgoldsimonStatusNone=>Works For Me
      Planned Release1.5.0=>
    Wed 19 Aug 2015 08:34:53 AM UTCgoldsimonPlanned Release=>1.5.0
    Mon 23 Mar 2015 03:54:09 PM UTCanewmanCarbon-Copy-=>Added anewman

    Back to the top


    Powered by Savane 3.1-cleanup1