Opened 16 months ago

Closed 8 months 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: 0.3.3.1-alpha
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

Description

panic="abort" needs to be set for all profiles in Cargo.toml, at least until the upstream bug is fixed: https://github.com/rust-lang/rust/issues/52652

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

Child Tickets

Change History (18)

comment:1 Changed 16 months ago by cyberpunks

See the rust-panic1 branch at ​​​​https://gitgud.io/onionk/tor.git

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

comment:2 Changed 16 months ago by nickm

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

comment:3 Changed 16 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 16 months ago by chelseakomlo (previous) (diff)

comment:4 Changed 16 months ago by chelseakomlo

Status: needs_reviewneeds_information

comment:5 Changed 16 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 9 months 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 9 months 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 9 months ago by teor

Keywords: 033-backport-unreached added

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

comment:9 Changed 8 months ago by elichai2

why not use catch_unwind() ?

comment:10 in reply to:  9 Changed 8 months 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:
https://gitgud.io/onionk/tor/tree/rust-panic1
(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 8 months ago by teor

Sponsor: SponsorV-can

comment:12 Changed 8 months ago by asn

Reviewer: teor

comment:13 Changed 8 months 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 8 months ago by teor

Status: needs_reviewmerge_ready
Version: Tor: 0.3.3.1-alpha

comment:15 Changed 8 months ago by asn

Keywords: asn-merge removed

merged to 040 and onwards.

comment:16 Changed 8 months ago by nickm

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

comment:17 Changed 8 months ago by teor

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

Drop the -alpha from backport tags

comment:18 Changed 8 months 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.