Opened 4 years ago

Last modified 2 years ago

#17101 needs_revision defect

Tests for connection_ap_handshake_rewrite_and_attach

Reported by: ipazmino Owned by: ipazmino
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: coverage, tor-tests-coverage, tor-tests-unit
Cc: Actual Points:
Parent ID: #17288 Points: 3
Reviewer: asn Sponsor: SponsorS-can

Description

Hi,

Here's a branch with some tests which cover the evaluation of an exit address at the connection_ap_handshake_rewrite_and_attach function in the connection_edge_ap_handshake.c file

https://github.com/twstrike/tor_for_patching/tree/connection_edge_handshake_tests

Child Tickets

Change History (26)

comment:1 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-final

comment:2 Changed 4 years ago by nickm

Sponsor: SponsorS

comment:3 Changed 4 years ago by nickm

Points: medium

comment:4 Changed 4 years ago by nickm

Severity: Normal
Status: newneeds_review

comment:5 Changed 4 years ago by nickm

Status: needs_reviewneeds_revision

Quick review:

  • Won't pass with --enable-gcc-warnings
  • Puts "http" in socks address, where it doesn't actually go.
  • The duplicated code in all these tests distracts from the actual intent of the test.

comment:6 Changed 4 years ago by ipazmino

Owner: set to ipazmino
Status: needs_revisionaccepted

I have made all the requested changes plus the correction of the valgrind warnings.
Everything is in the same branch.
https://github.com/twstrike/tor_for_patching/tree/connection_edge_handshake_tests

Please move this ticket to the adequate new state.

comment:7 Changed 4 years ago by teor

Status: acceptedneeds_review

comment:8 Changed 4 years ago by nickm

Keywords: merge-ready added
Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

Looks good, but it's going to need to get adapted to the new log-mocking stuff. That's going to take a little while, so I'll mark this to get merged early in 0.2.9.

comment:9 Changed 3 years ago by isabela

Sponsor: SponsorSSponsorS-can

comment:10 Changed 3 years ago by ipazmino

Status: needs_reviewaccepted

comment:11 Changed 3 years ago by ipazmino

Status: acceptedneeds_review

comment:12 Changed 3 years ago by nickm

Keywords: TorCoreTeam201604 added

Every postponed needs_review ticket should get a review in April

comment:13 Changed 3 years ago by nickm

Keywords: tor-tests-coverage tor-tests-unit added

comment:14 Changed 3 years ago by nickm

Keywords: TorCoreTeam201605 TorCoreTeam-postponed-201604 added; TorCoreTeam201604 removed

April is over; calling these april tickets postponed into may.

comment:15 Changed 3 years ago by asn

Reviewer: asn

comment:16 Changed 3 years ago by asn

Status: needs_reviewneeds_revision

Hello,

thanks for the code! Here is an initial review:

  • It seems like branch connection_edge_handshake_tests has the master branch merged into it, instead of it being rebased on top of the master branch. This will not apply cleanly on top of upstream master. I'd suggest you start with the master branch and then apply your changes on top of it. Or git-rebase master on top of your changes.
  • The conn_edge_ap_handshake/rewrite_and_attach_closes_conn_for_excluded_exit test seems to crash as follows:
    May 04 16:33:06.300 [err] tor_assertion_failed_(): Bug: src/common/container.c:1410: strmap_set: Assertion val failed; aborting. (on Tor 0.2.8.1-alpha-dev 9c6a817c53093831)
    May 04 16:33:06.300 [err] Bug: Assertion val failed in strmap_set at src/common/container.c:1410. Stack trace: (on Tor 0.2.8.1-alpha-dev 9c6a817c53093831)
    May 04 16:33:06.300 [err] Bug:     ./src/test/test(log_backtrace+0x52) [0x7fc4dd28a1c2] (on Tor 0.2.8.1-alpha-dev 9c6a817c53093831)
    May 04 16:33:06.300 [err] Bug:     ./src/test/test(tor_assertion_failed_+0xa4) [0x7fc4dd29eaa4] (on Tor 0.2.8.1-alpha-dev 9c6a817c53093831)
    May 04 16:33:06.300 [err] Bug:     ./src/test/test(strmap_set+0x197) [0x7fc4dd296fd7] (on Tor 0.2.8.1-alpha-dev 9c6a817c53093831)
    May 04 16:33:06.300 [err] Bug:     ./src/test/test(+0x204ab9) [0x7fc4dcf25ab9] (on Tor 0.2.8.1-alpha-dev 9c6a817c53093831)
    May 04 16:33:06.300 [err] Bug:     ./src/test/test(+0x3b87fa) [0x7fc4dd0d97fa] (on Tor 0.2.8.1-alpha-dev 9c6a817c53093831)
    May 04 16:33:06.300 [err] Bug:     ./src/test/test(testcase_run_one+0x165) [0x7fc4dd0d99c5] (on Tor 0.2.8.1-alpha-dev 9c6a817c53093831)
    May 04 16:33:06.300 [err] Bug:     ./src/test/test(tinytest_main+0x11e) [0x7fc4dd0da11e] (on Tor 0.2.8.1-alpha-dev 9c6a817c53093831)
    May 04 16:33:06.300 [err] Bug:     ./src/test/test(main+0x347) [0x7fc4dcddb8c7] (on Tor 0.2.8.1-alpha-dev 9c6a817c53093831)
    May 04 16:33:06.300 [err] Bug:     /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5) [0x7fc4db593b45] (on Tor 0.2.8.1-alpha-dev 9c6a817c53093831)
    May 04 16:33:06.300 [err] Bug:     ./src/test/test(+0xbc0c2) [0x7fc4dcddd0c2] (on Tor 0.2.8.1-alpha-dev 9c6a817c53093831)
    

The strmap_set() call seems to be the offending call here. Also what does:
routerset_t *excluded_nodes = excluded_nodes = routerset_new() do?
Let me know if you can't reproduce this and you need help debugging this further.

  • There seem to be some unnecessary whitespace changes in src/test/include.am.
  • There seem to be some printf calls left over in the tests.

comment:17 Changed 3 years ago by isabela

Points: medium3

comment:18 Changed 3 years ago by nickm

Keywords: TorCoreTeam201605 removed

Remove "TorCoreTeam201605" keyword. The time machine is broken.

comment:19 Changed 3 years ago by nickm

Parent ID: #17288

comment:20 Changed 3 years ago by nickm

Keywords: nickm-deferred-20160905 added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

Deferring many tickets that are in needs_revision with no progress noted for a while, where I think they could safely wait till 0.3.0 or later.

Please feel free to move these back to 0.2.9 if you finish the revisions soon.

comment:21 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:22 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:23 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:24 Changed 2 years ago by nickm

Keywords: nickm-deferred-20160905 removed

comment:25 Changed 2 years ago by nickm

Keywords: TorCoreTeam-postponed-201604 removed

comment:26 Changed 2 years ago by nickm

Keywords: merge-ready removed
Note: See TracTickets for help on using tickets.