[Date Prev][Date Next][Thread Prev][Thread Next]
- Subject: Re: Is the registry needed?
- From: Patrick Donnelly <batrick@...>
- Date: Fri, 2 Oct 2015 09:57:09 -0400
On Thu, Oct 1, 2015 at 7:28 PM, William Ahern
> 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 <email@example.com> 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.
> 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
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.