Re: Is the registry needed?

[Patrick Donnelly <batrick@...>]

On Thu, Oct 1, 2015 at 7:28 PM, William Ahern
<william@25thandclement.com> wrote:
> On Thu, Oct 01, 2015 at 11:55:18AM -0400, Sean Conner wrote:
>> It was thus said that the Great Patrick Donnelly once stated:
>> > On Wed, Sep 30, 2015 at 4:54 PM, Sean Conner <sean@conman.org> wrote:
>> > > It was thus said that the Great Soni L. once stated:
>> > >> This is VERY unsafe! You're potentially breaking a sandbox by doing a
>> > >> sethook. And you don't reset it, either.
>> > >
>> > >   If you mean "I don't call lua_sethook(L,NULL,0,0)" then you have misread
>> > > the code---I do call that.  But if you mean "I don't restore the previous
>> > > hooks" then you're right, I don't.  When I first started this implementation
>> > > of the signal module, I was concerned about that.  And I tried.  And it was
>> > > very fragile.  I then decided to check how other signal modules handled this
>> > > and guess what?  NONE OF THEM RESTORE THE HOOKS!  Not even the standalone
>> > > Lua interpeter bothers with restoring any hooks.
>> >
>> > https://github.com/batrick/lua-signal/blob/49b2fccf546ca0096af6aca26052eb0981e28f59/lsignal.c#L167-L211
>>
>>   Oh!  Well ... um ... okay.
>>
>
> That code seems broken to me.

Probably is.

> 1) hook processes outstanding signals
> 2) hook loads NULL from old_hook.hook at line 196
> 3) signal handler is invoked
> 4) handle loads NULL from old_hook.hook at line 203
> 5) handle calls lua_sethook at line 208
> 6) handle updates signal_stack at line 210
> 7) signal handler exits
> 8) hook invokes lua_sethook with NULL hook argument and exits
> 9) hook is never run again
>
> The obvious and simplest fix, I think, is to block all signals in the hook.

Yup, that and unconditionally using sigaction is in an upcoming update.

> The increments aren't atomic, even on x86. It's probably benign, but also
> unnecessary.

Why? A signal handler running for, say SIGINT, runs atomically for all
SIGINT signals. Because the count is maintained per signal, the
increments should be atomic.

> FWIW, l_pause is totally broken, but it looks like an afterthought, anyhow.
> If a signal arrives after entering l_pause but before calling sigprocmask,
> the process could hang. It should run any outstanding Lua hooks after
> calling sigprocmask. Also, it never restores the old signal mask.

Yes it was an afterthought.

-- 
Patrick Donnelly