buglwIP - A Lightweight TCP/IP stack - Bugs: bug #51593, Fix swapped src/dst params with...

 
 

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

bug #51593: Fix swapped src/dst params with ip4_route_src

Submitted by:  Joel Cunningham <jcunningham>
Submitted on:  Thu 27 Jul 2017 04:00:40 PM UTC  
 
Category: IPv4Severity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Joel Cunningham <jcunningham>
Open/Closed: ClosedPlanned Release: None
lwIP version: git head

(Jump to the original submission Jump to the original submission)

Mon 31 Jul 2017 02:05:13 PM UTC, comment #9:

Patch committed in 2b2ea50cb1b6e26a3d26eeca001529249afa1845. Task #14594 opened for API cleanup

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Sat 29 Jul 2017 05:12:13 AM UTC, comment #8:

Yes, of course!

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 28 Jul 2017 09:39:48 PM UTC, comment #7:

Just noticed that posted patch was based on a branch containing my port. Attached patch is ready for submission

Simon,

Are you ok with fixing the bug then addressing API changes (non-functional as David mentioned) later?

(file #41369)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 27 Jul 2017 09:52:19 PM UTC, comment #6:

> Just my 2c there


Excellent two cents, especially considering the revert case re-introducing the bug :)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 27 Jul 2017 09:33:18 PM UTC, comment #5:

Just my 2c there: I think that as a general software development principle, it is a good idea to push the bugfix immediately and separately from such a larger otherwise-nonfunctional API change in just about all cases. Not only may the API change take a while longer and thus needlessly leave the bug unsolved in the meantime, but also, consider for example what happens when later on, someone disagrees with the new order of the parameters and rolls back the API change commit they thought was purely nonfunctional, thereby reintroducing the bugs as well. I am guilty of violating that principle myself on many occasions, but I think it's a good idea nonetheless.. :)

David van Moolenbroek <dcvmoole>
Thu 27 Jul 2017 09:16:19 PM UTC, comment #4:

Thanks for the feedback, is the consensus to re-work the APIs to be less confusing or should I go ahead and push the patch?

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 27 Jul 2017 07:38:46 PM UTC, comment #3:

Agreed that it sucks :) I agree with your reasoning as well, Simon. Especially since eg ip4_route() takes only a destination, I think it would make more sense to have the source as an optional second parameter when looking at all routing functions at once. But any improvement to the overall consistency would be great!

David van Moolenbroek <dcvmoole>
Thu 27 Jul 2017 06:36:36 PM UTC, comment #2:

Hmm, this API really sucks :-)

I think the reason for the different src/dst order is that IPv6 had source as first and only later, source-based IPv4 routing was added. We should have made IPv6 routing take dst first and src second.

On the other hand, ip_route() is probably more often used, so we might just adopt the order in ip4_route_src?

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 27 Jul 2017 04:24:05 PM UTC, comment #1:

Hm yes, the UDP one is my bad, indeed for the reason you mention..

David van Moolenbroek <dcvmoole>
Thu 27 Jul 2017 04:00:40 PM UTC, original submission:

I found some occurrences of ip4_route_src() with the src, dst parameters in the wrong order. The UDP case was found through testing and the ICMP issue through code analysis of ip4_route_src

This appears to be a typo to me, most likely due to confusion with ip_route(src, dst) and ip4_route_src(dst, src).

More details about UDP issue:

LWIP_IPV4_SRC_ROUTING is 0. The UDP application socket was bound to INADDR_ANY with IP_MULTICAST_IF set. Transmits would result in calling ip4_route(dst) where dst was pcb->local_addr (which was INADDR_ANY) instead of pcb->mcast_ip4. This resulted in a routing failure

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.

 

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by dcvmoole (Posted a comment)
  • -unavailable- added by jcunningham (Submitted the item)
  •  

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

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 31 Jul 2017 02:05:13 PM UTCjcunninghamStatusNone=>Fixed
      Assigned toNone=>jcunningham
      Open/ClosedOpen=>Closed
    Fri 28 Jul 2017 09:39:48 PM UTCjcunninghamAttached File-=>Added 0001-ip4-fix-swapped-src-dst-params-with-ip4_route_src.patch, #41369
    Fri 28 Jul 2017 09:38:02 PM UTCjcunninghamAttached File#41362=>Removed
    Thu 27 Jul 2017 04:00:40 PM UTCjcunninghamAttached File-=>Added 0001-ip4-fix-swapped-src-dst-params-with-ip4_route_src.patch, #41362

    Back to the top


    Powered by Savane 3.1-cleanup1