Opened 4 months ago

Closed 3 months ago

#29668 closed defect (fixed)

Drop thread_fast_rng during postfork; improve thread_fast_rng fork-safety

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: asn-merge
Cc: Actual Points: .2
Parent ID: Points: .1
Reviewer: ahf Sponsor: SponsorV

Description

If you fork with a thread_fast_rng, we need to invalidate the thread_fast_rng so it will get reconstructed in the child process.

We should also tweak thread_fast_rng so that it is always misuse-resistant across fork()s in the same way. Right now it has 3 behaviors if you try to share it across a fork:

  • it works seamlessly if you have zero-on-fork support
  • it crashes if you have drop-on-fork support
  • it becomes an insecure prng if you have neither.

Child Tickets

Change History (12)

comment:1 Changed 4 months ago by nickm

Actual Points: .2
Reviewer: dgoulet

PR at https://github.com/torproject/tor/pull/761 ; the branch is ticket29668

comment:2 Changed 4 months ago by nickm

(I've done some tweaking, and now the tests pass again.)

comment:3 Changed 4 months ago by nickm

Sponsor: SponsorV

Calling this SponsorV because it's needed for #26288.

comment:4 Changed 4 months ago by nickm

Reviewer: dgoulet
Status: assignedneeds_review

comment:5 Changed 4 months ago by asn

Reviewer: catalyst

comment:6 Changed 4 months ago by nickm

Reviewer: catalystahf

comment:7 Changed 4 months ago by nickm

Priority: MediumHigh

comment:8 Changed 3 months ago by ahf

Status: needs_reviewmerge_ready

I think this looks good. There is one cosmetic detail that I tried to figure out on IRC: do we today prefer enum's over the three #define'd values in INHERIT_RES_*? If you think #define is preferred here then let's merge it.

comment:9 Changed 3 months ago by nickm

Hm. I don't have a strong preference but I guess an enum is a little better in theory. If CI passes, I'll squash and mark this for asn-merge.

comment:10 Changed 3 months ago by nickm

Keywords: asn-merge teor-merge added

CI passed; force-pushed a squashed version.

comment:11 Changed 3 months ago by nickm

Keywords: teor-merge removed

batch-modify: asn has offered to merge these tomorrow, so removing them from teor's plate. Teor -- you can merge these anyway if you are blocked on any of them and it would save you time to do so.

comment:12 Changed 3 months ago by asn

Resolution: fixed
Status: merge_readyclosed

merged to master!

Note: See TracTickets for help on using tickets.