tor_fragile_assert() when Unix domain socket is used

When a Unix domain socket is used for the SocksPort, a soft assertion is triggered inside tor_addr_compare_masked(). Log message: Sep 28 11:34:45.000 [warn] tor_bug_occurred_(): Bug: src/common/address.c:1119: tor_addr_compare_masked: This line should not have been reached. (Future instances of this warning will be silenced.) (on Tor 0.2.9.2-alpha 00ec701f8343f552)

The config used is: SocksPort unix:/Users/.../TorBrowser-Data/Tor/socks.socket IPv6Traffic PreferIPv6 KeepAliveIsolateSOCKSAuth

For more details, see: ticket:20111#comment:13

changed milestone to %Tor: 0.2.9.x-final

added component::core tor milestone::Tor: 0.2.9.x-final owner::yawning priority::high resolution::fixed severity::normal status::closed tbb-needs type::defect version::tor 0.2.9.2-alpha labels

Yawning says we can work around this by adding NoIsolateClientAddr to the SocksPort isolation flags.

Adding NoIsolateClientAddr did not fix the problem for me on OSX. I will attach a stack trace.

Trac:
stack.txt

stack trace (at the tor_fragile_assert() call)

The assert still happens because we always compare addresses regardless of if IsolateClientAddr is set.

This requires a few changes I think, after looking at it:

• We need to disable IsolateClientAddr whne using AF_LOCAL sockets (Easy, done in my branch).
• tor_addr_compare_masked() should probably handle AF_LOCAL sockets.

Trac:
Status: new to assigned

Putting this as High because tbb-needs.

Trac:
Priority: Medium to High

Trac:
Milestone: N/A to Tor: 0.2.9.x-final

This is ugly because we never bothered to treat AF_UNIX as a full class citizen. In particular tor_addr_t doesn't contain the path at all, so when we go to compare them, there isn't any information regarding the address beyond the family.

The correct solution is probably to extend tor_addr_t to Do The Right Thing, but this branch should cover Tor Browser's use cases. The first commit is probably safe to take since it's the IsolateClientAddr change, the second may be more controversial.

https://git.schwanenlied.me/yawning/tor/src/bug20261

Trac:
Status: assigned to needs_review

Looks good to me, but I have one question.

In case there are other places in the code --or there later become places in the code! -- where we compare two AF_UNIX addresses, maybe we should make them turn out unequal by default? I think that missing a match is probably safer than reporting a spurious match, right? If you agree, I'll switch the hack to do a pointer comparison.

(Also, a note on tor_addr_t : the main reason to use tor_addr_t at all, instead of sockaddr_storage, was to keep it small in the case where we need to create a zillion of them. So if we're going to make AF_UNIX firstclass, we'll need some other approach to keeping it small.)

2e7e635c593f13 tweaks this a little as discussed above. Merged!

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

My tests with Tor Browser show that these changes fix the problem. Thank you Nick and Yawning!

I encountered another one of these when testing tor master with Tor Browser: Oct 28 10:38:31.000 [warn] tor_bug_occurred_: Bug: src/or/connection_edge.c:1593: connection_ap_handshake_rewrite_and_attach: This line should not have been reached. (Future instances of this warning will be silenced.) (on Tor 0.3.0.0-alpha-dev f3e158edf7d8128d)

I will attach the stack trace that was logged. Should I open a new ticket or should I reopen this one?

Trac:
stack2.txt

logged stack trace

Replying to mcs:

I encountered another one of these when testing tor master with Tor Browser: Oct 28 10:38:31.000 [warn] tor_bug_occurred_: Bug: src/or/connection_edge.c:1593: connection_ap_handshake_rewrite_and_attach: This line should not have been reached. (Future instances of this warning will be silenced.) (on Tor 0.3.0.0-alpha-dev f3e158edf7d8128d)

I will attach the stack trace that was logged. Should I open a new ticket or should I reopen this one?

Looks like a completely different issue. Please open a new ticket. (I'm not sure why that assertion is even there.)

Replying to teor:

Looks like a completely different issue. Please open a new ticket. (I'm not sure why that assertion is even there.)

Thanks for the quick feedback. I created #20494 (moved).

closed

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