Implement clean_up_backtrace_handler()

Split off legacy/trac#31594 (moved):

clean_up_backtrace_handler() doesn't do anything, but it should:

• disable backtrace signal handlers
• destroy the backtrace mutex (regression to legacy/trac#21788 (moved))

changed milestone to %Tor: 0.4.1.x-final in legacy/trac

Trac:
Child Ticket(s): legacy/trac#31736 (moved), legacy/trac#31594 (moved), legacy/trac#31734 (moved)

added 041-backport-maybe in Legacy / Trac 042-should in Legacy / Trac BugSmashFund in Legacy / Trac actualpoints::0.4 in Legacy / Trac component::core tor/tor in Legacy / Trac consider-backport-after-042-stable in Legacy / Trac diagnostics in Legacy / Trac milestone::Tor: 0.4.1.x-final in Legacy / Trac owner::teor in Legacy / Trac points::0.2 in Legacy / Trac priority::medium in Legacy / Trac regression in Legacy / Trac resolution::fixed in Legacy / Trac reviewer::nickm in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac type::defect in Legacy / Trac version::tor 0.3.5.1-alpha in Legacy / Trac labels

See my pull request:

• 0.3.5: https://github.com/torproject/tor/pull/1302

The merge forward was clean.

Here are the test branches for merging forwards:

• ​​https://github.com/teor2345/tor/branches/all?query=bug31614_

Trac:
Version: N/A to Tor: 0.3.5.1-alpha
Keywords: android, macos deleted, BugSmash added
Cc: gaba to N/A
Points: N/A to 0.2
Status: new to needs_review
Actualpoints: N/A to 0.2

Trac:
Keywords: N/A deleted, regression added
Description: Part of legacy/trac#31594 (moved)

to

Split off legacy/trac#31594 (moved):

clean_up_backtrace_handler() doesn't do anything, but it should:

• disable backtrace signal handlers
• destroy the backtrace mutex (regression to legacy/trac#21788 (moved))

Trac:
Keywords: 041-backport, 040-backport, 035-backport deleted, 040-backport-maybe, 035-backport-maybe, 041-backport-maybe added

I don't think we should backport this fix, unless the bug is actually causing issues in older versions.

Trac:
Keywords: N/A deleted, consider-backport-after-042-stable, consider-backport-if-needed added

Trac:
Owner: N/A to teor
Status: needs_review to assigned

Trac:
Status: assigned to needs_review

Fix bug smash fund spelling

Trac:
Keywords: BugSmash deleted, BugSmashFund added

Trac:
Reviewer: N/A to dgoulet

This ticket is independent of its parent now.

Trac:
Parent: legacy/trac#31594 (moved) to N/A

Question that might result in no change but still there. One tiny nitpick also.

Trac:
Status: needs_review to needs_revision

You're right, there are a bunch of nasty race conditions here.

This is what we need to fix:

1. Always lock the mutex before using cb_buf (there are some places where we don't do this)
2. Lock the mutex before adding or removing signal handlers (might not be strictly required, but it makes reasoning about locks easier)
3. Abort if destroying the mutex fails (doesn't save us from undefined behaviour, but does notify us when it happens)

We could also 4. Lock a mutex before logging anywhere in the err module (this could cause more issues than it solves)

The best way to do this might be to put cb_buf in its own file, and use accessor functions.

I removed the commit that added undefined behaviour, and edited the changes file.

Please review my PR:

• 0.3.5: https://github.com/torproject/tor/pull/1302

The merge forward is clean, the test branches are here:

• https://github.com/teor2345/tor/branches/all?query=bug31614_

Trac:
Status: needs_revision to needs_review

Trac:
Actualpoints: 0.2 to 0.4

Trac:
Reviewer: dgoulet to nickm

This mostly LGTM. Before merging, let's think make a conscious decision about what we should do about restart testing (see discussion on legacy/trac#31735 (moved)).

Also, I worry that this could interfere with our sandbox code: I think that in order to allow us to call sigaction() on all of these values, we might need to add them to sb_rt_sigaction() in sandbox.c.

Please feel free to merge-ready once the above issues are taken care of.

[edited to include actual ticket number]

Trac:
Status: needs_review to needs_revision

Replying to nickm:

This mostly LGTM. Before merging, let's think make a conscious decision about what we should do about restart testing (see discussion on legacy/trac#31735 (moved)).

(I think you meant legacy/trac#31736 (moved) - the quick fix mutex ticket.)

I think we should leave restart testing for legacy/trac#31735 (moved) - the eventual fix mutex ticket. I'm out of time on this issue, and the changes are smaller than I expected.

Also, I worry that this could interfere with our sandbox code: I think that in order to allow us to call sigaction() on all of these values, we might need to add them to sb_rt_sigaction() in sandbox.c.

Yes, the sandbox would also need to use these values after a re-initialise.

I made the following changes:

• added the signals to the sandbox
• rewrote one of the commits based on legacy/trac#31736 (moved)
• rebased on the latest maint-0.3.5 to fix a test-stem error

Here is the PR:

• 0.3.5: https://github.com/torproject/tor/pull/1302

The merge forward is clean.

Trac:
Status: needs_revision to needs_review

Trac:
Status: needs_review to merge_ready

Nick, do you think we should backport the commit that destroys the backtrace signal handler? Otherwise we'll trigger undefined behaviour when Tor is re-initialised. (But I'm not sure if we should backport, it seems risky.)

Hm. I think we should hold ourselves ready to do a backport, but not do one unless we actually run into problems in practice.

Sure. Apparently that was my opinion about this whole branch 2 weeks ago: consider-backport-if-needed

I was wrong about the backtrace buffer mutex: we can't destroy it, because it is statically allocated.

I removed the call to mutex_destroy(), and explained why in a comment.

All the other commits are unchanged.

Trac:
Status: merge_ready to needs_review

I also added a comment explaining why it's ok to not destroy the log mutex.

I think this is okay.

Trac:
Status: needs_review to merge_ready
Keywords: N/A deleted, asn-merge added

Merged. Leaving open for potential backports?

Trac:
Keywords: asn-merge deleted, N/A added
Milestone: Tor: 0.4.2.x-final to Tor: 0.4.1.x-final

Merged to 0.4.1. Marking for consideration for further backport.

Trac:
Milestone: Tor: 0.4.1.x-final to Tor: 0.4.0.x-final

I'm leaning towards "no backport" on this ticket, unless we discover a specific bug. Leaving open, so we check again after 042-stable.

This change doesn't seem to make much of a difference, still thinking "no backport".

Similar code has caused some bugs, and 0.4.0 is obsolete, so I'm going to close these tickets without backport.

Trac:
Milestone: Tor: 0.4.0.x-final to Tor: 0.4.1.x-final
Resolution: N/A to fixed
Keywords: 040-backport-maybe, 035-backport-maybe, consider-backport-if-needed deleted, N/A added
Status: merge_ready to closed

closed

changed time estimate to 1h 36m

added 3h 12m of time spent

mentioned in issue legacy/trac#31734 (moved)

mentioned in issue legacy/trac#31735 (moved)

mentioned in issue legacy/trac#31736 (moved)

mentioned in issue legacy/trac#31594 (moved)

moved from legacy/trac#31614 (moved)

added Bug label and removed 1 deleted label

added Regression label and removed 1 deleted label

added BugSmashFund label