HomePhabricator

[Signal] Allow one-shot SIGPIPE handler to be reached
Audit Required9a3f892d0182

Authored by vsk on Dec 4 2019, 7:21 PM.

Description

[Signal] Allow one-shot SIGPIPE handler to be reached

As SIGPIPE is no longer in the IntSigs array, handle SIGPIPE before
handling any interrupt signals.

Thanks to Alexandre Ganea for pointing out the issue here.

Details

Auditors
rnk
Committed
vskDec 4 2019, 7:38 PM
Parents
rG1de214fa413d: [fix][unittests][llvm] Fix running unit tests without assertions. [NFCI]
Branches
Unknown
Tags
Unknown

Event Timeline

aganea added an auditor: rnk.Dec 5 2019, 6:57 AM
aganea added a subscriber: aganea.

Thanks Vedant! Minor: there's still an unexpected side-effect when InitLLVM is called with InstallPipeSignalExitHandler=false or if sys::SetOneShotPipeSignalFunction is used to clear the default function. In that case, the test will fall through, and will go and call llvm::sys::RunSignalHandlers(). Whereas before it used raise the signal again, and return. What do you think?
I'm wondering if the code shouldn't read more like:

if (llvm::is_contained(IntSigs, Sig) || Sig == SIGPIPE) {
  if (auto OldInterruptFunction = InterruptFunction.exchange(nullptr))
    return OldInterruptFunction();

  if (Sig == SIGPIPE)
    if (auto OldOneShotPipeFunction =
            OneShotPipeSignalFunction.exchange(nullptr))
      return OldOneShotPipeFunction();

  raise(Sig); // Execute the default handler.
  return;
}
This commit now requires audit.Dec 5 2019, 6:57 AM
vsk added a comment.Jan 8 2021, 9:48 AM

Thanks Vedant! Minor: there's still an unexpected side-effect when InitLLVM is called with InstallPipeSignalExitHandler=false or if sys::SetOneShotPipeSignalFunction is used to clear the default function. In that case, the test will fall through, and will go and call llvm::sys::RunSignalHandlers(). Whereas before it used raise the signal again, and return. What do you think?
I'm wondering if the code shouldn't read more like:

if (llvm::is_contained(IntSigs, Sig) || Sig == SIGPIPE) {
  if (auto OldInterruptFunction = InterruptFunction.exchange(nullptr))
    return OldInterruptFunction();

  if (Sig == SIGPIPE)
    if (auto OldOneShotPipeFunction =
            OneShotPipeSignalFunction.exchange(nullptr))
      return OldOneShotPipeFunction();

  raise(Sig); // Execute the default handler.
  return;
}

@aganea I genuinely apologize for losing track of your feedback. I didn't realize I had missed this until Duncan pinged me, after the recent commentary on D70277.

there's still an unexpected side-effect when InitLLVM is called with InstallPipeSignalExitHandler=false or if sys::SetOneShotPipeSignalFunction is used to clear the default function. In that case, the test will fall through, and will go and call llvm::sys::RunSignalHandlers(). Whereas before it used raise the signal again, and return. What do you think?

That's the correct assessment. RunSignalHandlers runs handlers added by AddSignalHandler, which is for "when an abort/kill signal is delivered to the process": that's not a great fit for SIGPIPE. OTOH, your proposed change would do away with a nice guarantee made in the SetOneShopPipeSignalHandler documentation, which states: "The LLVM signal handling code will not install any handler for the pipe signal unless one is provided with this API". Wdyt of re-raising the signal when the SIGPIPE/IntSigs handlers are missing?:

if (Sig == SIGPIPE)
  if (auto OldOneShotPipeFunction =
          OneShotPipeSignalFunction.exchange(nullptr))
    return OldOneShotPipeFunction();

bool IsIntSig = llvm::is_contained(IntSigs, Sig);
if (IsIntSig)
  if (auto OldInterruptFunction = InterruptFunction.exchange(nullptr))
    return OldInterruptFunction();

if (Sig == SIGPIPE || IsIntSig) {
  raise(Sig);   // Execute the default handler.
  return;
}

FWIW, that logic look corrects to me.

Seems good to me too! Thank you!

vsk added a comment.Jan 8 2021, 11:14 AM

Thanks, I've landed this change as bf401256edd0.