Re: [tor-bugs] #30721 [Core Tor/Tor]: tor_addr_port_lookup() is overly permissive

[Tor Bug Tracker & Wiki]

#30721: tor_addr_port_lookup() is overly permissive
-+-
 Reporter:  teor |  Owner:  teor
 Type:  defect   | Status:  closed
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  unspecified
 Severity:  Normal   | Resolution:  fixed
 Keywords:  technical-debt, tor-addr, refactor,  |  Actual Points:  1.5
  practracker-improvement|
Parent ID:   | Points:  0.5
 Reviewer:  catalyst |Sponsor:
 |  Sponsor31-can
-+-
Changes (by nickm):

 * status:  merge_ready => closed
 * resolution:   => fixed


Comment:

 Squashed and merged!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #30721 [Core Tor/Tor]: tor_addr_port_lookup() is overly permissive

[Tor Bug Tracker & Wiki]

#30721: tor_addr_port_lookup() is overly permissive
-+-
 Reporter:  teor |  Owner:  teor
 Type:  defect   | Status:
 |  merge_ready
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  unspecified
 Severity:  Normal   | Resolution:
 Keywords:  technical-debt, tor-addr, refactor,  |  Actual Points:  1.5
  practracker-improvement|
Parent ID:   | Points:  0.5
 Reviewer:  catalyst |Sponsor:
 |  Sponsor31-can
-+-

Comment (by teor):

 Replying to [comment:9 catalyst]:
 > Replying to [comment:8 teor]:
 > > Replying to [comment:7 catalyst]:
 > > > Thanks! The new tests look reasonable. I wonder if it's necessary to
 make all those multi-statement macro definitions, instead of helper
 functions (and maybe much smaller macros that call those functions)? I
 think helper function would be a little cleaner. Please let me know what
 you think.
 > >
 > > The problem with helper functions is that failures are attributed to
 the test assertions in the helper function, without any context. So it's
 very hard to tell which test data caused the failure.
 > >
 > > I'm not sure if there is some way of providing context as part of the
 test macros. I suggest that we merge this code as-is, because it's
 functional, adds coverage, and makes sure we won't introduce future bugs.
 Then we can open a ticket for follow-up.
 > I see. I agree we should open a new ticket to follow up, in that case.
 >
 > I think I see that the problem is buried in the `TT_DECLARE()` macro in
 `tinytest_macros.h`. I think it's possible to work around it, but it might
 be nontrivial. (Rough sketch: redefine `TT_DECLARE()` in helper functions
 to read file and line info from function parameters, and make file and
 line numbers explicit parameters to helper functions.)

 I opened #30968 with your suggestion, and my alternate suggestion (a
 format string that allows us to print the data being tested).

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #30721 [Core Tor/Tor]: tor_addr_port_lookup() is overly permissive

[Tor Bug Tracker & Wiki]

#30721: tor_addr_port_lookup() is overly permissive
-+-
 Reporter:  teor |  Owner:  teor
 Type:  defect   | Status:
 |  merge_ready
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  unspecified
 Severity:  Normal   | Resolution:
 Keywords:  technical-debt, tor-addr, refactor,  |  Actual Points:  1.5
  practracker-improvement|
Parent ID:   | Points:  0.5
 Reviewer:  catalyst |Sponsor:
 |  Sponsor31-can
-+-
Changes (by catalyst):

 * status:  needs_review => merge_ready


Comment:

 Replying to [comment:8 teor]:
 > Replying to [comment:7 catalyst]:
 > > Thanks! The new tests look reasonable. I wonder if it's necessary to
 make all those multi-statement macro definitions, instead of helper
 functions (and maybe much smaller macros that call those functions)? I
 think helper function would be a little cleaner. Please let me know what
 you think.
 >
 > The problem with helper functions is that failures are attributed to the
 test assertions in the helper function, without any context. So it's very
 hard to tell which test data caused the failure.
 >
 > I'm not sure if there is some way of providing context as part of the
 test macros. I suggest that we merge this code as-is, because it's
 functional, adds coverage, and makes sure we won't introduce future bugs.
 Then we can open a ticket for follow-up.
 I see. I agree we should open a new ticket to follow up, in that case.

 I think I see that the problem is buried in the `TT_DECLARE()` macro in
 `tinytest_macros.h`. I think it's possible to work around it, but it might
 be nontrivial. (Rough sketch: redefine `TT_DECLARE()` in helper functions
 to read file and line info from function parameters, and make file and
 line numbers explicit parameters to helper functions.)

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #30721 [Core Tor/Tor]: tor_addr_port_lookup() is overly permissive

[Tor Bug Tracker & Wiki]

#30721: tor_addr_port_lookup() is overly permissive
-+-
 Reporter:  teor |  Owner:  teor
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  unspecified
 Severity:  Normal   | Resolution:
 Keywords:  technical-debt, tor-addr, refactor,  |  Actual Points:  1.5
  practracker-improvement|
Parent ID:   | Points:  0.5
 Reviewer:  catalyst |Sponsor:
 |  Sponsor31-can
-+-
Changes (by teor):

 * status:  needs_information => needs_review


Comment:

 Replying to [comment:7 catalyst]:
 > Thanks! The new tests look reasonable. I wonder if it's necessary to
 make all those multi-statement macro definitions, instead of helper
 functions (and maybe much smaller macros that call those functions)? I
 think helper function would be a little cleaner. Please let me know what
 you think.

 The problem with helper functions is that failures are attributed to the
 test assertions in the helper function, without any context. So it's very
 hard to tell which test data caused the failure.

 I'm not sure if there is some way of providing context as part of the test
 macros. I suggest that we merge this code as-is, because it's functional,
 adds coverage, and makes sure we won't introduce future bugs. Then we can
 open a ticket for follow-up.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #30721 [Core Tor/Tor]: tor_addr_port_lookup() is overly permissive

[Tor Bug Tracker & Wiki]

#30721: tor_addr_port_lookup() is overly permissive
-+-
 Reporter:  teor |  Owner:  teor
 Type:  defect   | Status:
 |  needs_information
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  unspecified
 Severity:  Normal   | Resolution:
 Keywords:  technical-debt, tor-addr, refactor,  |  Actual Points:  1.5
  practracker-improvement|
Parent ID:   | Points:  0.5
 Reviewer:  catalyst |Sponsor:
 |  Sponsor31-can
-+-
Changes (by catalyst):

 * status:  needs_review => needs_information


Comment:

 Thanks! The new tests look reasonable. I wonder if it's necessary to make
 all those multi-statement macro definitions, instead of helper functions
 (and maybe much smaller macros that call those functions)? I think helper
 function would be a little cleaner. Please let me know what you think.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #30721 [Core Tor/Tor]: tor_addr_port_lookup() is overly permissive

[Tor Bug Tracker & Wiki]

#30721: tor_addr_port_lookup() is overly permissive
-+-
 Reporter:  teor |  Owner:  teor
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  unspecified
 Severity:  Normal   | Resolution:
 Keywords:  technical-debt, tor-addr, refactor,  |  Actual Points:  1.5
  practracker-improvement|
Parent ID:   | Points:  0.5
 Reviewer:  catalyst |Sponsor:
 |  Sponsor31-can
-+-
Changes (by teor):

 * status:  needs_revision => needs_review
 * actualpoints:  1.0 => 1.5


Comment:

 Replying to [comment:5 teor]:
 > Replying to [comment:4 catalyst]:
 > > * Maybe add unit tests to ensure that IPv4 addresses with square
 brackets get rejected?
 >
 > Hmm yeah the unit tests are not in a great state. I'm working on them.

 So I found two bugs while improving the unit tests:
 * ambiguous IPv6:port without brackets were allowed, now they are banned
 in the address-port parsing functions only (1b39bde)
 * sometimes the port was set on failure, now it is always zero (679cce7)

 I rewrote the tests and added a lot of tests cases, to make sure we
 covered all these changes.

 Some of the ambiguous test cases may fail on Linux or Windows (I only
 tested on macOS), so I'll check CI in an hour or so to make sure.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #30721 [Core Tor/Tor]: tor_addr_port_lookup() is overly permissive

[Tor Bug Tracker & Wiki]

#30721: tor_addr_port_lookup() is overly permissive
-+-
 Reporter:  teor |  Owner:  teor
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  unspecified
 Severity:  Normal   | Resolution:
 Keywords:  technical-debt, tor-addr, refactor,  |  Actual Points:  1.0
  practracker-improvement|
Parent ID:   | Points:  0.5
 Reviewer:  catalyst |Sponsor:
 |  Sponsor31-can
-+-
Changes (by teor):

 * actualpoints:  0.5 => 1.0


Comment:

 Replying to [comment:4 catalyst]:
 > Replying to [comment:1 teor]:
 > > This bug was introduced in in 0.2.1.5-alpha, when tor_addr_lookup()
 was called tor_addr_port_parse().
 > >
 > > The first commit fixes the bug, the next two commits refactor the code
 so the logic is clearer. I split tor_addr_lookup() into 3 separate
 functions as part of the refactor, the split gets rid of a practracker
 exception.
 > >
 > > See my pull request https://github.com/torproject/tor/pull/1068
 > >
 > > This change will break some rare, invalid tor configs, so we can't
 backport it.
 > Thanks! This looks good by visual inspection. The commit structure is
 helpful. The first commit could use a few minor changes:
 > * Add a changes file

 The PR already has changes/bug30721:
 https://github.com/torproject/tor/pull/1068/files#diff-
 82ae46251bd81539f5fb75c1d7e7a82b

 > * Maybe add unit tests to ensure that IPv4 addresses with square
 brackets get rejected?

 Hmm yeah the unit tests are not in a great state. I'm working on them.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #30721 [Core Tor/Tor]: tor_addr_port_lookup() is overly permissive

[Tor Bug Tracker & Wiki]

#30721: tor_addr_port_lookup() is overly permissive
-+-
 Reporter:  teor |  Owner:  teor
 Type:  defect   | Status:
 |  needs_revision
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  unspecified
 Severity:  Normal   | Resolution:
 Keywords:  technical-debt, tor-addr, refactor,  |  Actual Points:  0.5
  practracker-improvement|
Parent ID:   | Points:  0.5
 Reviewer:  catalyst |Sponsor:
 |  Sponsor31-can
-+-
Changes (by catalyst):

 * status:  needs_review => needs_revision


Comment:

 Replying to [comment:1 teor]:
 > This bug was introduced in in 0.2.1.5-alpha, when tor_addr_lookup() was
 called tor_addr_port_parse().
 >
 > The first commit fixes the bug, the next two commits refactor the code
 so the logic is clearer. I split tor_addr_lookup() into 3 separate
 functions as part of the refactor, the split gets rid of a practracker
 exception.
 >
 > See my pull request https://github.com/torproject/tor/pull/1068
 >
 > This change will break some rare, invalid tor configs, so we can't
 backport it.
 Thanks! This looks good by visual inspection. The commit structure is
 helpful. The first commit could use a few minor changes:
 * Add a changes file
 * Maybe add unit tests to ensure that IPv4 addresses with square brackets
 get rejected?

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #30721 [Core Tor/Tor]: tor_addr_port_lookup() is overly permissive

[Tor Bug Tracker & Wiki]

#30721: tor_addr_port_lookup() is overly permissive
-+-
 Reporter:  teor |  Owner:  teor
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  unspecified
 Severity:  Normal   | Resolution:
 Keywords:  technical-debt, tor-addr, refactor,  |  Actual Points:  0.5
  practracker-improvement|
Parent ID:   | Points:  0.5
 Reviewer:  catalyst |Sponsor:
 |  Sponsor31-can
-+-
Changes (by asn):

 * reviewer:   => catalyst


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #30721 [Core Tor/Tor]: tor_addr_port_lookup() is overly permissive

[Tor Bug Tracker & Wiki]

#30721: tor_addr_port_lookup() is overly permissive
--+
 Reporter:  teor  |  Owner:  teor
 Type:  defect| Status:  needs_review
 Priority:  Medium|  Milestone:  Tor: 0.4.2.x-final
Component:  Core Tor/Tor  |Version:  Tor: unspecified
 Severity:  Normal| Resolution:
 Keywords:  technical-debt, tor-addr  |  Actual Points:  0.5
Parent ID:| Points:  0.5
 Reviewer:|Sponsor:  Sponsor31-can
--+
Changes (by teor):

 * status:  assigned => needs_review
 * keywords:  technical-debt, operator-visible-change => technical-debt,
 tor-addr
 * points:  0.2 => 0.5
 * version:   => Tor: unspecified
 * actualpoints:  0.2 => 0.5


Comment:

 This bug was introduced in in 0.2.1.5-alpha, when tor_addr_lookup() was
 called tor_addr_port_parse().

 The first commit fixes the bug, the next two commits refactor the code so
 the logic is clearer. I split tor_addr_lookup() into 3 separate functions
 as part of the refactor, the split gets rid of a practracker exception.

 See my pull request https://github.com/torproject/tor/pull/1068

 This change will break some rare, invalid tor configs, so we can't
 backport it.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #30721 [Core Tor/Tor]: tor_addr_port_lookup() is overly permissive

[Tor Bug Tracker & Wiki]

#30721: tor_addr_port_lookup() is overly permissive
-+-
 Reporter:  teor |  Owner:  teor
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.4.2.x-final
Component:  Core Tor/Tor |Version:  Tor:
 |  unspecified
 Severity:  Normal   | Resolution:
 Keywords:  technical-debt, tor-addr, refactor,  |  Actual Points:  0.5
  practracker-improvement|
Parent ID:   | Points:  0.5
 Reviewer:   |Sponsor:
 |  Sponsor31-can
-+-
Changes (by teor):

 * keywords:  technical-debt, tor-addr => technical-debt, tor-addr,
 refactor, practracker-improvement


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

[tor-bugs] #30721 [Core Tor/Tor]: tor_addr_port_lookup() is overly permissive

[Tor Bug Tracker & Wiki]

#30721: tor_addr_port_lookup() is overly permissive
-+-
 Reporter:  teor |  Owner:  teor
 Type:  defect   | Status:  assigned
 Priority:  Medium   |  Milestone:  Tor: 0.4.2.x-final
Component:  Core |Version:
  Tor/Tor|   Keywords:  technical-debt, operator-visible-
 Severity:  Normal   |  change
Actual Points:  0.2  |  Parent ID:
   Points:  0.2  |   Reviewer:
  Sponsor:   |
  Sponsor31-can  |
-+-
 Like tor_addr_parse() in #23082, tor_addr_port_lookup() also allows square
 brackets around IPv4 addresses.

 But it's slightly less permissive: it requires a terminating `]`.

 For example, this command line should be rejected, but it is not:
 {{{
 tor ORPort [127.0.0.1]:9001
 }}}

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs