[Date Prev][Date Next][Thread Prev][Thread Next]
- Subject: Re: Is the registry needed?
- From: William Ahern <william@...>
- Date: Thu, 1 Oct 2015 16:28:51 -0700
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.
At a minimum I would call sigfillset on the sigaction mask at line 292.
Analysis is needlessly complicated by multiple signals.
The increments aren't atomic, even on x86. It's probably benign, but also
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.