Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not allocate or unwind after fork #81858

Merged
merged 14 commits into from
May 16, 2021
Merged

Commits on May 7, 2021

  1. std panicking: Make decrease() return ()

    Nothing uses the return value.  This will make the next changes
    easier.
    
    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    ijackson committed May 7, 2021
    Configuration menu
    Copy the full SHA
    a9f43a2 View commit details
    Browse the repository at this point in the history
  2. std panicking: Provide panic::always_abort

    We must change the atomic read on panic entry to `Acquire`, to pick up
    a possible an `always_panic` on another thread.
    
    We add `count` to the names of panic_count::get and ::is_zaero,
    because now there is another reason why panic ought to maybe abort.
    Renaming these ensures that we have checked every call site to ensure
    that they don't need further adjustment.
    
    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    Co-authored-by: Mara Bos <m-ou.se@m-ou.se>
    ijackson and m-ou-se committed May 7, 2021
    Configuration menu
    Copy the full SHA
    1b1bf24 View commit details
    Browse the repository at this point in the history
  3. std panicking: ALWAYS_ABORT: use Relaxed memory ordering

    As per
      rust-lang#81858 (comment)
    
    Suggested-by: Mara Bos <m-ou.se@m-ou.se>
    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    ijackson committed May 7, 2021
    Configuration menu
    Copy the full SHA
    3cba120 View commit details
    Browse the repository at this point in the history
  4. panic/fork: Command: Do not unwind after fork() in child

    Unwinding after fork() in the child is UB on some platforms, because
    on those (including musl) malloc can be UB in the child of a
    multithreaded program, and unwinding must box for the payload.
    
    Even if it's safe, unwinding past fork() in the child causes whatever
    traps the unwind to return twice.  This is very strange and clearly
    not desirable.  With the default behaviour of the thread library, this
    can even result in a panic in the child being transformed into zero
    exit status (ie, success) as seen in the parent!
    
    Fixes rust-lang#79740.
    
    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    ijackson committed May 7, 2021
    Configuration menu
    Copy the full SHA
    820123a View commit details
    Browse the repository at this point in the history
  5. unix process: pre_exec: Discuss panic safety

    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    Co-authored-by: Mara Bos <m-ou.se@m-ou.se>
    ijackson and m-ou-se committed May 7, 2021
    Configuration menu
    Copy the full SHA
    9283cdc View commit details
    Browse the repository at this point in the history
  6. panic tests: Command: Test that we do not unwind past fork

    This is safe (does not involve heap allocation) but we don't yet have
    a test to ensure that stays true.  That will come in a moment.
    
    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    Co-authored-by: Mara Bos <m-ou.se@m-ou.se>
    ijackson and m-ou-se committed May 7, 2021
    Configuration menu
    Copy the full SHA
    f801506 View commit details
    Browse the repository at this point in the history
  7. panic ui test: Provide comprehensive test for panic after fork

    This tests that we can indeed safely panic after fork, both
    a raw libc::fork and in a Command pre_exec hook.
    
    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    Co-authored-by: Mara Bos <m-ou.se@m-ou.se>
    ijackson and m-ou-se committed May 7, 2021
    Configuration menu
    Copy the full SHA
    a17eab7 View commit details
    Browse the repository at this point in the history
  8. panic ui test: Improve error handling

    Previoously, if somehow this program got a wrong argument, it would
    panic in the re-executed child.  But that looks like a "success"
    for this program!  We mustn't panic unless everything is great.
    
    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    ijackson committed May 7, 2021
    Configuration menu
    Copy the full SHA
    19429ce View commit details
    Browse the repository at this point in the history
  9. panic ui test: Add a test for panic::always_abort

    Our existing tests are only on Unix.  We want a general one too.
    
    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    ijackson committed May 7, 2021
    Configuration menu
    Copy the full SHA
    12fe500 View commit details
    Browse the repository at this point in the history
  10. panic ui test: Test always_abort on one thread, panic on another

    This test failed on an earlier version of this branch, where this did
    not work properly, so I know the test works.
    
    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    ijackson committed May 7, 2021
    Configuration menu
    Copy the full SHA
    756771d View commit details
    Browse the repository at this point in the history
  11. panic/fork test: Do not run on emscripten

    fork fails there.  The failure message is confusing: so c.status()
    returns an Err, the closure panics, and the test thinks the panic was
    propagated from inside the child.
    
    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    Co-authored-by: Mara Bos <m-ou.se@m-ou.se>
    ijackson and m-ou-se committed May 7, 2021
    Configuration menu
    Copy the full SHA
    8220f2f View commit details
    Browse the repository at this point in the history

Commits on May 13, 2021

  1. Use SIGUSR1 rather than SIGTRAP for "allocated after fork"

    Some platforma (eg ARM64) apparently generate SIGTRAP for panic abort!
    
    See eg
      rust-lang#81858 (comment)
    
    This is probably a bug, but (i) we want to avoid that bug rather than
    trying to fix it now and (ii) it would better to use a signal that is
    less at risk of strangeness.
    
    I grepped the rust-lang/rut codebase for SIGUSR and there were no hits.
    
    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    ijackson committed May 13, 2021
    Configuration menu
    Copy the full SHA
    f6a4963 View commit details
    Browse the repository at this point in the history
  2. Tolerate SIGTRAP for panic abort after panic::always_abort

    Some platforma (eg ARM64) apparently generate SIGTRAP for panic abort!
    
    See eg
      rust-lang#81858 (comment)
    
    This is probably a bug, but we don't want to entangle this MR with it.
    When it's fixed, this commit should be reverted.
    
    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    ijackson committed May 13, 2021
    Configuration menu
    Copy the full SHA
    6369637 View commit details
    Browse the repository at this point in the history

Commits on May 14, 2021

  1. panic abort after fork test: Disable on android

    And link to the issue.
    
    Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
    ijackson committed May 14, 2021
    Configuration menu
    Copy the full SHA
    88ccaa7 View commit details
    Browse the repository at this point in the history