Opened 13 months ago

Closed 3 months ago

#19926 closed defect (user disappeared)

BUG warning in connection_ap_attach_pending: waiting for rendezvous desc :*

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.9.1-alpha
Severity: Normal Keywords: bug, regression?
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

connection_ap_attach_pending: Bug: 0x613000b8d680 is no longer in circuit_wait. Its current state is waiting for rendezvous desc. Why is it on pending_entry_connections? (on Tor 0.2.9.1-alpha f6c7e131a1ceb178)
Yeah, why?

Child Tickets

Change History (18)

comment:1 Changed 13 months ago by teor

Keywords: bug regression? added
Milestone: Tor: 0.2.9.x-final

Thanks for letting us know about this bug. We're working on fixing these up.

/** A list of all the entry_connection_t * objects that are not marked
 * for close, and are in AP_CONN_STATE_CIRCUIT_WAIT.
 *
 * (Right now, we check in several places to make sure that this list is
 * correct.  When it's incorrect, we'll fix it, and log a BUG message.)
 */
static smartlist_t *pending_entry_connections = NULL;

comment:2 Changed 13 months ago by nickm

Summary: waiting for rendezvous desc :*BUG warning in connection_ap_attach_pending: waiting for rendezvous desc :*

comment:3 Changed 12 months ago by nickm

Actual Points: .1
Owner: set to nickm
Status: newaccepted

I think that the problem lies in the test in conmnection_exit_connect(). The parentheses are wrong. We should allow rendezvous connections to IPv6 addresses even when IPv6Exit is false, right?

I've tried rewriting this test to be correct and easier to read; please review and test my branch bug19926?

Last edited 12 months ago by nickm (previous) (diff)

comment:4 Changed 12 months ago by nickm

Status: acceptedneeds_review

comment:5 Changed 12 months ago by nickm

Owner: nickm deleted
Status: needs_reviewassigned

oh darn that was the wrong ticket. Please ignore previous messages.

comment:6 Changed 12 months ago by nickm

Status: assignednew

comment:7 Changed 11 months ago by dgoulet

Actual Points: .1

@teor, did we fixed your comment:1 ? If not, which ticket does this referred to?

comment:8 Changed 11 months ago by arma

Have we audited all the cases where a stream can go from AP_CONN_STATE_CIRCUIT_WAIT to AP_CONN_STATE_RENDDESC_WAIT, to make sure that we frob pending_entry_connections appropriately in all of them?

comment:9 in reply to:  7 Changed 11 months ago by teor

Replying to dgoulet:

@teor, did we fixed your comment:1 ? If not, which ticket does this referred to?

I have no idea, I just read the comment in the code.

comment:10 in reply to:  8 ; Changed 11 months ago by arma

Replying to arma:

Have we audited all the cases where a stream can go from AP_CONN_STATE_CIRCUIT_WAIT to AP_CONN_STATE_RENDDESC_WAIT, to make sure that we frob pending_entry_connections appropriately in all of them?

There are four places where we transition from some state into AP_CONN_STATE_RENDDESC_WAIT: in circuit_get_open_circ_or_launch() in circuituse.c, in connection_ap_handshake_rewrite_and_attach() in connection_edge.c, in rend_client_send_introduction() in rendclient.c, and in rend_client_report_intro_point_failure() in rendclient.c. But in all four of these cases, we call connection_ap_mark_as_non_pending_circuit() right before. So I think we should be ok there.

Next, have we audited all the cases where we add a stream to pending_entry_connections, to make sure we aren't in state AP_CONN_STATE_RENDDESC_WAIT when we do it?

comment:11 in reply to:  10 Changed 11 months ago by arma

Replying to arma:

have we audited all the cases where we add a stream to pending_entry_connections, to make sure we aren't in state AP_CONN_STATE_RENDDESC_WAIT when we do it?

There are two places where a stream can get added onto pending_entry_connections:

1) In connection_ap_mark_as_pending_circuit(), which does a tor_assert to make sure that the state is AP_CONN_STATE_CIRCUIT_WAIT.

2) In connection_ap_attach_pending(), which only does it if the state is AP_CONN_STATE_CIRCUIT_WAIT.

So I don't think that can be it either.

(I should clarify that the above analysis is looking at git branch f6c7e131a, like in the original report.)

I am now out of places to look.

Git commit 0a701e53 looks like it fixed a few bugs like this, but as far as I can tell, that went into 0.2.8.1-alpha, so way before 0.2.9.1-alpha.

I wonder if there are any cases where we call connection_ap_mark_as_non_pending_circuit() with the wrong pointer, e.g. that we mis-cast it because we're confused about our OO tricks? Then that function silently doesn't remove anything, leaving us in the situation where the stream is still on pending_entry_connections?

Maybe Nick has other guesses, based on debugging past bugs on this pending_entry_connections thing?

comment:12 Changed 10 months ago by nickm

For 0.3.0, I would suggest that we refactor everything that changes an AP_CONN's state, and everything that changes the pending_entry_connections list, to make sure they cannot be wrong.

For 0.2.9, that seems kinda big. Maybe we should just tune this warning down a little bit in 0.2.9?

comment:13 Changed 10 months ago by dgoulet

At this point of 029 in rc and almost stable, I would agree on warning down the log but still this should stay on our stack for 030.

comment:14 Changed 10 months ago by nickm

Status: newneeds_review

bug19926_029 downgrades the warning to info, for 0.2.9.

I'm also wondering whether we have any places where we confuse this connection for another type of connection, and set its state to some other "6" other than AP_CONN_STATE_RENDDESC_WAIT. But I can't find any place like that, after hunting seriously for a while.

If we turn DEBUGGING_17659 on again, that might help?

comment:15 Changed 10 months ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

comment:16 Changed 9 months ago by nickm

Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final
Status: merge_readyneeds_information

Merged to 0.2.9; not taken in master. Thanks! I'm moving this ticket to maint-0.3.0 and needs_information, with the question being: Does this occur with 0.3.0 or later?

comment:17 Changed 8 months ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: unspecified

5 weeks with no updates.

comment:18 Changed 3 months ago by nickm

Resolution: user disappeared
Status: needs_informationclosed
Note: See TracTickets for help on using tickets.