Opened 8 months ago

Closed 2 days ago

#27199 closed defect (fixed)

panic inside rust extern "C" function is undefined behavior

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor:
Severity: Normal Keywords: nickm-merge, rust, 034-backport, 035-backport, 040-backport, 041-proposed, 033-backport-unreached, consider-backport-after-0404
Cc: Actual Points: 0.1
Parent ID: Points: 0.1
Reviewer: teor Sponsor: SponsorV-can


panic="abort" needs to be set for all profiles in Cargo.toml, at least until the upstream bug is fixed:

This is already set for [profile.release] builds, but not for the others.

Child Tickets

Change History (18)

comment:1 Changed 8 months ago by cyberpunks

See the rust-panic1 branch at ​​​​

Last edited 8 months ago by cyberpunks (previous) (diff)

comment:2 Changed 8 months ago by nickm

Milestone: Tor: 0.3.5.x-final
Status: newneeds_review

comment:3 Changed 8 months ago by chelseakomlo

This change looks ok to me (I can't think of a use case when we would want to allow for unwinding as opposed to aborting).

Did you manually test this change with the different builds (test, bench, etc) to ensure that the project still builds successfully?

Last edited 8 months ago by chelseakomlo (previous) (diff)

comment:4 Changed 8 months ago by chelseakomlo

Status: needs_reviewneeds_information

comment:5 Changed 8 months ago by chelseakomlo

It would be helpful for these PRs to have Travis run against them so we can see how this impacts tor builds- I'm waiting for feedback from other network team members if there is a non-github alternative, but ideally with these PRs we could also see the build status.

comment:6 Changed 7 weeks ago by teor

Keywords: 041-proposed added
Milestone: Tor: 0.3.5.x-finalTor: unspecified

We might need this change as we add more Rust code.

comment:7 Changed 7 weeks ago by teor

Keywords: 033-backport removed

These open, non-merge_ready tickets can not get backported to 0.3.3, because 0.3.3 is now unsupported.

comment:8 Changed 7 weeks ago by teor

Keywords: 033-backport-unreached added

Hmm, I guess they should still get 033-backport-unreached

comment:9 Changed 3 weeks ago by elichai2

why not use catch_unwind() ?

comment:10 in reply to:  9 Changed 3 weeks ago by teor

Keywords: 035-backport 040-backport added
Milestone: Tor: unspecifiedTor: 0.4.1.x-final
Status: needs_informationneeds_review

Replying to elichai2:

why not use catch_unwind() ?

We would have to remember to wrap every C to Rust FFI call in catch_unwind(), which would make our code hard to read.
(The codebase is meant to be panic-free, but we might have missed some panics.)

Instead, we should merge the abort branch in 0.4.1 and see how we go:
(If we abort, we should get a nice backtrace.)

We need to:

  • open a GitHub pull request so we get CI
  • review the code
  • merge it

comment:11 Changed 3 weeks ago by teor

Sponsor: SponsorV-can

comment:12 Changed 13 days ago by asn

Reviewer: teor

comment:13 Changed 6 days ago by teor

Actual Points: 0.1
Keywords: asn-merge nickm-merge consider-backport-after-0404-alpha added
Points: 0.1

Here are the pull requests:

I'll merge PR 945 to maint-0.3.4, and PR 947 to maint-0.3.5. This change seems safe enough to backport in the next backport group.

Please merge PR 947 to maint-0.4.0 and merge forward to master.

comment:14 Changed 6 days ago by teor

Status: needs_reviewmerge_ready
Version: Tor:

comment:15 Changed 6 days ago by asn

Keywords: asn-merge removed

merged to 040 and onwards.

comment:16 Changed 5 days ago by nickm

Milestone: Tor: 0.4.1.x-finalTor: 0.3.5.x-final

comment:17 Changed 4 days ago by teor

Keywords: consider-backport-after-0404 added; consider-backport-after-0404-alpha removed

Drop the -alpha from backport tags

comment:18 Changed 2 days ago by teor

Milestone: Tor: 0.3.5.x-finalTor: 0.3.4.x-final
Resolution: fixed
Status: merge_readyclosed

Merged to 0.3.4, 0.3.5, and merged forward.

Merged #23790, #29665, #29017, #27199, #29144, #13221, #28698.

Note: See TracTickets for help on using tickets.