Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19960 closed defect (fixed)

Tor v0.2.9.1-alpha-dev (git-b3f43a22ab921ce6) - failing options/validate__transproxy test on NetBSD

Reported by: yancm Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: 0.2.9.1-alpha
Severity: Normal Keywords: self test regression netbsd
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by arma)

Latest tor dev sources fail self test.

# ./src/or/tor -
Aug 22 20:14:56.657 [notice] Tor v0.2.9.1-alpha-dev (git-b3f43a22ab921ce6) running on NetBSD with Libevent 2.1.5-beta, OpenSSL 1.0.2h and Zlib 1.2.3.
NetBSD 6_Stable on i386

******
 1/758 TESTS FAILED. (2 skipped)
 gmake: * * *  [Makefile:7666: test] Error 1

******
 options/validate__transproxy: [forking]
  FAIL src/test/test_options.c:1105: assert(tdata)
  [validate__transproxy FAILED]

Child Tickets

Attachments (3)

0001-Use-the-correct-preprocessor-macro-for-Linux.patch (1.6 KB) - added by cypherpunks 3 years ago.
DebugOutput20161109.rar (473.1 KB) - added by yancm 3 years ago.
test --debug (output 20161110)
0001-Use-the-correct-preprocessor-macro-for-Linux.2.patch (2.1 KB) - added by cypherpunks 3 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 3 years ago by nickm

Keywords: regression netbsd added
Milestone: Tor: unspecifiedTor: 0.2.9.x-final

Do you know how to use "git bisect" ? If so, it would be really helpful to know when this bug first appeared.

comment:2 Changed 3 years ago by cypherpunks

I don't have a NetBSD machine available, but I'm guessing it has something to do with #19449. Maybe the change enabled transparent proxying by default on NetBSD where it was previously disabled?

Looking at the test (https://gitweb.torproject.org/tor.git/tree/src/test/test_options.c?id=a3d419634bef0bbac4118d08272d59c9fe66a1bb#n1082) there isn't a case that tests NetBSD leading to the failed assertion.

I'll submit a patch that cleans up that part of the code to improve readability and fix the incorrect preprocessor variable for detecting Linux platforms (linux instead of __linux__). Because i don't have a NetBSD machine I'm unsure which firewall system is used on NetBSD. It seems the rest of the Tor code assumes NetBSD is similar to OpenBSD...so i guess it should use pf-divert?

comment:3 in reply to:  1 Changed 3 years ago by yancm

Replying to nickm:

Do you know how to use "git bisect" ? If so, it would be really helpful to know
when this bug first appeared.

Sorry, I do not. I also got out of my habit of daily builds so cannot help that way either.

comment:4 in reply to:  2 ; Changed 3 years ago by yancm

Replying to cypherpunks:

I don't have a NetBSD machine available,

[...]

Because i don't have a NetBSD machine I'm unsure which firewall system is used on NetBSD.
It seems the rest of the Tor code assumes NetBSD is similar to OpenBSD...so i guess it
should use pf-divert?

I'm uncertain, it might be built in. I actually use ipf (pf and npf are also available)

I'll try your patch soon unless it's already been pushed up...

comment:5 in reply to:  4 Changed 3 years ago by cypherpunks

Replying to yancm:

Replying to cypherpunks:

I don't have a NetBSD machine available,

[...]

Because i don't have a NetBSD machine I'm unsure which firewall system is used on NetBSD.
It seems the rest of the Tor code assumes NetBSD is similar to OpenBSD...so i guess it
should use pf-divert?

I'm uncertain, it might be built in. I actually use ipf (pf and npf are also available)

I'll try your patch soon unless it's already been pushed up...

I don't expect the patch to solve your issue. It was just meant as premature cleanup of related code before actually fixing the issue.

comment:6 Changed 3 years ago by nickm

Summary: Tor v0.2.9.1-alpha-dev (git-b3f43a22ab921ce6) - failing one test on NetBSDTor v0.2.9.1-alpha-dev (git-b3f43a22ab921ce6) - failing options/validate__transproxy test on NetBSD

comment:7 Changed 3 years ago by arma

Hey, does Tor commit ff3e08f2 have anything to do with this? It's about NetBSD and it's right next to a line about transproxy. Maybe there is someplace in the real code that is missing an || defined(__NetBSD__) entry?

I couldn't figure out from the commit whether there's a trac ticket that goes with it though.

comment:8 Changed 3 years ago by teor

The options tests are unfortunately quite fragile, because they depend on the exact log output. (For example, they could be failing simply because the platform name is different.)

It might help to have a unit test log at a useful verbosity level.
Try src/test/test --debug or similar.

Changed 3 years ago by yancm

Attachment: DebugOutput20161109.rar added

test --debug (output 20161110)

comment:9 Changed 3 years ago by yancm

I've attached debug output above. I've noticed a couple of other test errors I have not yet reported.

comment:10 Changed 3 years ago by teor

I split off #20634 for the unnecessary address/get_if_addrs6_list_no_internal failure.

There does seem to be some variation in the results - have you changed versions or applied the attached patch since running the tests?

Because this is what your logs contain:

options/validate__transproxy: [forking] Nov 10 14:47:57.490 [warn] Bad Scheduler
LowWaterMark__ option

  FAIL src/test/test_options.c:1086: assert(!msg)
  [validate__transproxy FAILED]

And this was what you originally reported:

options/validate__transproxy: [forking]

    FAIL src/test/test_options.c:1105: assert(tdata)
    [validate__transproxy FAILED]
]}}

comment:11 Changed 3 years ago by yancm

I believe I had tried applying the patch 3 months back. Since then I have continued tracking the latest tor sources, so the debug output is against the latest git HEAD version - Tor 0.3.0.0-alpha-dev (git-0980787f91cfc420).

comment:12 Changed 3 years ago by arma

Description: modified (diff)

comment:13 Changed 3 years ago by arma

Worth doing in 0.2.9.x if somebody can track down the issue. But it doesn't need to block a release candidate -- especially if we think there's a good chance that it's a bug in our unit tests, rather than a bug in actual Tor code?

comment:14 in reply to:  13 Changed 3 years ago by teor

Replying to arma:

Worth doing in 0.2.9.x if somebody can track down the issue. But it doesn't need to block a release candidate -- especially if we think there's a good chance that it's a bug in our unit tests, rather than a bug in actual Tor code?

Given it's in an options_validate test, on a non-Linux/macOS/Windows platform, it is almost certainly a bug in the unit tests. It could be as simple as expecting support for a feature on a platform where it does not exist.

However, it could also be a bug in that platform's implementation of the feature.

Either way, no reason to hold up stable.

comment:15 Changed 3 years ago by nickm

In e7ade23f9723f55a504ebd8279cbadf0aca108c6 I increased the verborsity of the output of the test, and now on the buildbot I see:

  FAIL src/test/test_options.c:1089: Expected NULL but got 'ipfw is a FreeBSD-specificand OS X/Darwin-specific feature.'

a) "specificand" is a typo.

b) Does netbsd support ipfw or not? We imply "yes" in test_options.c, but "no" in our definition of KERNEL_MAY_SUPPORT_IPFW. Which of these is correct?

comment:16 Changed 3 years ago by nickm

I think my branch bug19960_2 is the fix here. I've merged it to master. If it makes the builders happy, it can go into 0.2.9 as well.

comment:17 Changed 3 years ago by nickm

Status: newneeds_review

comment:18 Changed 3 years ago by cypherpunks

I've rebased my patch onto bug19960_2 which addresses the points in comment:2.

comment:19 in reply to:  15 Changed 3 years ago by yancm

Replying to nickm:

b) Does netbsd support ipfw or not? We imply "yes" in test_options.c, but "no" in our definition of KERNEL_MAY_SUPPORT_IPFW. Which of these is correct?

I am pretty sure the answer is no. NetBSD has some support for pf (OpenBSD origins), ipf (independent project? Darren Reed author - no longer under development) and npf which is the new official mechanism in NetBSD as I understand.

I've never used pf, I currently still use ipf only because too lazy to re-visit my rule-sets and replace scripts, etc.

comment:20 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

LGTM; merged that too.

comment:21 Changed 3 years ago by yancm

I've confirmed latest source compile [0.3.0.0-alpha-dev (git-f2445fc608fcb71c)] tests clean. I did not back up and test 0.2.9.0-alpha-dev as I am tracking only active dev source head.

comment:22 Changed 3 years ago by nickm

great; thanks for checking!

Note: See TracTickets for help on using tickets.