tor_addr_port_lookup() is overly permissive

changed milestone to %Tor: 0.4.2.x-final

added actualpoints::1.5 component::core tor/tor milestone::Tor: 0.4.2.x-final owner::teor points::0.5 practracker-improvement priority::medium refactor resolution::fixed reviewer::catalyst severity::normal sponsor::31-can status::closed technical-debt tor-addr type::defect version::tor unspecified labels

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.

Trac:
Actualpoints: 0.2 to 0.5
Version: N/A to Tor: unspecified
Points: 0.2 to 0.5
Keywords: operator-visible-change deleted, tor-addr added
Status: assigned to needs_review

Trac:
Keywords: N/A deleted, practracker-improvement, refactor added

Trac:
Reviewer: N/A to catalyst

Replying to 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?

Trac:
Status: needs_review to needs_revision

Replying to catalyst:

Replying to 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.

Trac:
Actualpoints: 0.5 to 1.0

Replying to teor:

Replying to 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.

Trac:
Status: needs_revision to needs_review
Actualpoints: 1.0 to 1.5

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.

Trac:
Status: needs_review to needs_information

Replying to 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.

Trac:
Status: needs_information to needs_review

Replying to teor:

Replying to 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.)

Trac:
Status: needs_review to merge_ready

Replying to catalyst:

Replying to teor:

Replying to 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 (moved) with your suggestion, and my alternate suggestion (a format string that allows us to print the data being tested).

Squashed and merged!

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

closed

changed time estimate to 4h

added 12h of time spent

mentioned in issue #30968 (moved)

mentioned in issue #32314 (moved)

moved to tpo/core/tor#30721 (closed)

mentioned in issue tpo/core/tor#30968 (closed)

tor_addr_port_lookup() is overly permissive

Child items 0

Activity