Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#20261 closed defect (fixed)

tor_fragile_assert() when Unix domain socket is used

Reported by: mcs Owned by: yawning
Priority: High Milestone: Tor: 0.2.9.x-final
Component: Core Tor Version: Tor: 0.2.9.2-alpha
Severity: Normal Keywords: tbb-needs
Cc: teor, brade, gk, arthuredelstein, adrelanos Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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

Child Tickets

Attachments (2)

stack.txt (2.2 KB) - added by mcs 4 years ago.
stack trace (at the tor_fragile_assert() call)
stack2.txt (3.4 KB) - added by mcs 4 years ago.
logged stack trace

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by mcs

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

Last edited 4 years ago by mcs (previous) (diff)

comment:2 Changed 4 years ago by mcs

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

Changed 4 years ago by mcs

Attachment: stack.txt added

stack trace (at the tor_fragile_assert() call)

comment:3 Changed 4 years ago by yawning

Status: newassigned

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.

comment:4 Changed 4 years ago by nickm

Priority: MediumHigh

Putting this as High because tbb-needs.

comment:5 Changed 4 years ago by nickm

Milestone: Tor: 0.2.9.x-final

comment:6 Changed 4 years ago by yawning

Status: assignedneeds_review

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

comment:7 Changed 4 years ago by nickm

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

comment:8 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

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

comment:9 Changed 4 years ago by mcs

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

comment:10 Changed 4 years ago by 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?

Changed 4 years ago by mcs

Attachment: stack2.txt added

logged stack trace

comment:11 in reply to:  10 ; Changed 4 years ago by teor

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

comment:12 in reply to:  11 Changed 4 years ago by mcs

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.

Note: See TracTickets for help on using tickets.