A crash in Lua 5.3.2

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

A crash in Lua 5.3.2

actboy168@gmail.com
```
local mt = {}
mt.__newindex = mt
local t = setmetatable({}, mt)
t[1] = 1
```



Reply | Threaded
Open this post in threaded view
|

Re: A crash in Lua 5.3.2

Roberto Ierusalimschy
> ```
> local mt = {}
> mt.__newindex = mt
> local t = setmetatable({}, mt)
> t[1] = 1
> ```

Right :-(  Thanks for the report. Probably it was introduced with the
optimizations on table operations; it does not happen in 5.3.1.

-- Roberto

Reply | Threaded
Open this post in threaded view
|

Re: A crash in Lua 5.3.2

Roberto Ierusalimschy
> > ```
> > local mt = {}
> > mt.__newindex = mt
> > local t = setmetatable({}, mt)
> > t[1] = 1
> > ```
>
> Right :-(  Thanks for the report. Probably it was introduced with the
> optimizations on table operations; it does not happen in 5.3.1.

The bug is quite subtle. Follows a fix:

diff for lvm.c:
----------------------------------------
193c193,194
<       lua_assert(ttistable(t) && ttisnil(oldval));
---
>       Table *h = hvalue(t);
>       lua_assert(ttisnil(oldval));
195c196
<       if ((tm = fasttm(L, hvalue(t)->metatable, TM_NEWINDEX)) == NULL &&
---
>       if ((tm = fasttm(L, h->metatable, TM_NEWINDEX)) == NULL &&
200c201
<          (oldval = luaH_newkey(L, hvalue(t), key), 1))) {
---
>          (oldval = luaH_newkey(L, h, key), 1))) {
203,204c204,205
<         invalidateTMcache(hvalue(t));
<         luaC_barrierback(L, hvalue(t), val);
---
>         invalidateTMcache(h);
>         luaC_barrierback(L, h, val);
----------------------------------------

-- Roberto

Reply | Threaded
Open this post in threaded view
|

Re: A crash in Lua 5.3.2

Coda Highland
On Fri, Jan 1, 2016 at 6:30 AM, Roberto Ierusalimschy
<[hidden email]> wrote:

>> > ```
>> > local mt = {}
>> > mt.__newindex = mt
>> > local t = setmetatable({}, mt)
>> > t[1] = 1
>> > ```
>>
>> Right :-(  Thanks for the report. Probably it was introduced with the
>> optimizations on table operations; it does not happen in 5.3.1.
>
> The bug is quite subtle. Follows a fix:
>
> diff for lvm.c:
> ----------------------------------------
> 193c193,194
> <       lua_assert(ttistable(t) && ttisnil(oldval));
> ---
> >       Table *h = hvalue(t);
> >       lua_assert(ttisnil(oldval));
> 195c196
> <       if ((tm = fasttm(L, hvalue(t)->metatable, TM_NEWINDEX)) == NULL &&
> ---
> >       if ((tm = fasttm(L, h->metatable, TM_NEWINDEX)) == NULL &&
> 200c201
> <          (oldval = luaH_newkey(L, hvalue(t), key), 1))) {
> ---
> >          (oldval = luaH_newkey(L, h, key), 1))) {
> 203,204c204,205
> <         invalidateTMcache(hvalue(t));
> <         luaC_barrierback(L, hvalue(t), val);
> ---
> >         invalidateTMcache(h);
> >         luaC_barrierback(L, h, val);
> ----------------------------------------
>
> -- Roberto
>

Oh my, that IS subtle. Good fix!

/s/ Adam

Reply | Threaded
Open this post in threaded view
|

Re: A crash in Lua 5.3.2

Egor Skriptunoff-2
On Fri, Jan 1, 2016 at 8:09 PM, Coda Highland <[hidden email]> wrote:
On Fri, Jan 1, 2016 at 6:30 AM, Roberto Ierusalimschy
<[hidden email]> wrote:
> The bug is quite subtle. Follows a fix:
>
> diff for lvm.c:
> ----------------------------------------
> 193c193,194
> <       lua_assert(ttistable(t) && ttisnil(oldval));
> ---
> >       Table *h = hvalue(t);
> >       lua_assert(ttisnil(oldval));
> 195c196
> <       if ((tm = fasttm(L, hvalue(t)->metatable, TM_NEWINDEX)) == NULL &&
> ---
> >       if ((tm = fasttm(L, h->metatable, TM_NEWINDEX)) == NULL &&
> 200c201
> <          (oldval = luaH_newkey(L, hvalue(t), key), 1))) {
> ---
> >          (oldval = luaH_newkey(L, h, key), 1))) {
> 203,204c204,205
> <         invalidateTMcache(hvalue(t));
> <         luaC_barrierback(L, hvalue(t), val);
> ---
> >         invalidateTMcache(h);
> >         luaC_barrierback(L, h, val);

that IS subtle

Does the bug caused by GC, unexpectedly changing the "gc" field, hence the hvalue(t) is not a constant?

Reply | Threaded
Open this post in threaded view
|

Re: A crash in Lua 5.3.2

Roberto Ierusalimschy
> > Does the bug caused by GC, unexpectedly changing the "gc" field, hence the
> hvalue(t) is not a constant?

No.

The bug is in luaV_finishset (lvm.c:187). Rememeber the program:

local MT = {}
MT.__newindex = MT
local T = setmetatable({}, MT)
T[1] = 1

(I changed the variable names to upercase because luaV_finishset has
a C variable named 't', too.)

When Lua executes T[1] = 1 and T[1] has a nil value, it goes into
luaV_finishset. There, it sees the metatable's __newindex field, and so
't' now points to this entry (MT.__newindex) in MT table, which is equal
to 't' (because MT.__newindex == MT). Then, in line 200, we have this:

           (oldval = luaH_newkey(L, hvalue(t), key), 1))) {

This opens space to insert the key '1' in table 't', which is MT.
As MT does not have that space, it is rehashed. In this rehash,
the field MT.__newindex (like any other field) is moved to a new
array, and the old array is deleted. But 't' is still pointing
into that old array! Then in line 203, we try to get the value
of 't' and so we end up accessing a deleted array.

-- Roberto

Reply | Threaded
Open this post in threaded view
|

Re: A crash in Lua 5.3.2

Lorenzo Donati-3
In reply to this post by Roberto Ierusalimschy


On 01/01/2016 15:30, Roberto Ierusalimschy wrote:

>>> ```
>>> local mt = {}
>>> mt.__newindex = mt
>>> local t = setmetatable({}, mt)
>>> t[1] = 1
>>> ```
>>
>> Right :-(  Thanks for the report. Probably it was introduced with the
>> optimizations on table operations; it does not happen in 5.3.1.
>
> The bug is quite subtle. Follows a fix:
>
Yep! Nice analysis!

Although it is subtle, it seems easy to trigger in Lua code. Should this
bug require a "fast-track" emergency bug-fix release of 5.3.3 ?

[...]

> -- Roberto
>
>

Cheers!

-- Lorenzo

Reply | Threaded
Open this post in threaded view
|

Re: A crash in Lua 5.3.2

Dirk Laurie-2
2016-01-05 19:18 GMT+02:00 Lorenzo Donati <[hidden email]>:

>
>
> On 01/01/2016 15:30, Roberto Ierusalimschy wrote:
>>>>
>>>> ```
>>>> local mt = {}
>>>> mt.__newindex = mt
>>>> local t = setmetatable({}, mt)
>>>> t[1] = 1
>> The bug is quite subtle. Follows a fix:

> Although it is subtle, it seems easy to trigger in Lua code.

Do you have an example that does not involve making
__newindex be the metatable itself?

Reply | Threaded
Open this post in threaded view
|

Re: A crash in Lua 5.3.2

Lorenzo Donati-3


On 05/01/2016 21:12, Dirk Laurie wrote:

> 2016-01-05 19:18 GMT+02:00 Lorenzo Donati <[hidden email]>:
>>
>>
>> On 01/01/2016 15:30, Roberto Ierusalimschy wrote:
>>>>>
>>>>> ```
>>>>> local mt = {}
>>>>> mt.__newindex = mt
>>>>> local t = setmetatable({}, mt)
>>>>> t[1] = 1
>>> The bug is quite subtle. Follows a fix:
>
>> Although it is subtle, it seems easy to trigger in Lua code.
>
> Do you have an example that does not involve making
> __newindex be the metatable itself?
>
>
No. Should I?
I said "it seems", so I was asking for clarification.

For "easy" I meant that triggering the crash doesn't need complicated
instruction sequences or weird values fed to some esoteric function, but
just operations a mildly experienced Lua programmer may legitimately try
out.

Using the metatable as target for newindex? Well, I cannot say if it is
a widespread technique in some weird OO framework, or what could be
useful for (too tired now), but it doesn't seem so weird that someone
honestly couldn't find a legitimate use for it.

I stand to be corrected, though; constant use of Lua made me reconsider
a lot of my OO habits of my Java/C++ past, so I use a more basic OO
approach now and tend to shun heavy OO contraptions. Therefore I'm a bit
"rusted" on what are "legitimate/useful" metatable tricks.

OTOH, please, keep in mind that we are not talking about a simple
misbehavior of the Lua engine, but a straightaway crash, so a real
showstopper and a potential security risk.

Having the latest Lua source with such a bug in it is not something
particularly good, IMHO. At least from a "marketing" POV.

This is especially true for new users, with little experience with C and
unable to apply the patch suggested by Roberto. And this is more
important because the standard distro is source only, so a newbie cannot
simply download a patched binary and be happy with it.

My 2eurocent, anyway.

Cheers!

-- Lorenzo




Reply | Threaded
Open this post in threaded view
|

Re: A crash in Lua 5.3.2

Dirk Laurie-2
2016-01-05 23:06 GMT+02:00 Lorenzo Donati <[hidden email]>:

>
>
> On 05/01/2016 21:12, Dirk Laurie wrote:
>>
>> 2016-01-05 19:18 GMT+02:00 Lorenzo Donati <[hidden email]>:
>>>
>>>
>>>
>>> On 01/01/2016 15:30, Roberto Ierusalimschy wrote:
>>>>>>
>>>>>>
>>>>>> ```
>>>>>> local mt = {}
>>>>>> mt.__newindex = mt
>>>>>> local t = setmetatable({}, mt)
>>>>>> t[1] = 1
>>>>
>>>> The bug is quite subtle. Follows a fix:
>>
>>
>>> Although it is subtle, it seems easy to trigger in Lua code.
>>
>>
>> Do you have an example that does not involve making
>> __newindex be the metatable itself?
>>
>>
> No. Should I?
> I said "it seems", so I was asking for clarification.
>
> For "easy" I meant that triggering the crash doesn't need complicated
> instruction sequences or weird values fed to some esoteric function, but
> just operations a mildly experienced Lua programmer may legitimately try
> out.
>
> Using the metatable as target for newindex? Well, I cannot say if it is a
> widespread technique in some weird OO framework, or what could be useful for
> (too tired now), but it doesn't seem so weird that someone honestly couldn't
> find a legitimate use for it.
>
> I stand to be corrected, though; constant use of Lua made me reconsider a
> lot of my OO habits of my Java/C++ past, so I use a more basic OO approach
> now and tend to shun heavy OO contraptions. Therefore I'm a bit "rusted" on
> what are "legitimate/useful" metatable tricks.
>
> OTOH, please, keep in mind that we are not talking about a simple
> misbehavior of the Lua engine, but a straightaway crash, so a real
> showstopper and a potential security risk.
>
> Having the latest Lua source with such a bug in it is not something
> particularly good, IMHO. At least from a "marketing" POV.
>
> This is especially true for new users, with little experience with C and
> unable to apply the patch suggested by Roberto. And this is more important
> because the standard distro is source only, so a newbie cannot simply
> download a patched binary and be happy with it.

As usual with Lua bugs, the bug and its fix have been reported
on http://www.lua.org/bugs.html. The description of the bug is:

   Metatable may access its own deallocated field when
   it has a self reference in __newindex.

The question asked was:  Should this bug require a "fast-track"
emergency bug-fix release of 5.3.3?

Lorenzo has ably summarized the reasons why it should, most
of which would be applicable to any bug. About the only one that
is unusual is that the fault shows up as a segfault, not a stack
overflow or some other error that can be trapped at the Lua level.

There are also reasons why it would be unusual to put out an
emergency bug-fix release of a new minor version, most of which
would be applicable to any bug.

1. The bug might not manifest itself on all machines.
2. The fix may introduce a new bug.
3. The combination of 1 and 2 would imply that an existing
working Lua version on that machine has needlessy been
broken.

The Lua 5.3.1 bug "io.lines does not check maximum number
of options" also causes a segfault on my machine. The code
to trigger it is also nothing esoteric. Yet 5.3.2-rc1 was only released
four months later the bug was reported.

I don't think the self-referencing __newindex bug is any worse,
certainly not emergency material.

However, the word "patches" among the quick links on the Download
page
   http://www.lua.org/download.html
is perhaps a little cryptic. It would be helpful, in the situations that
Lorenzo has highlighted, to have a sentence explaining that bugs
(if any) reported after the release was frozen can be found there.

Reply | Threaded
Open this post in threaded view
|

Re: A crash in Lua 5.3.2

Lorenzo Donati-3


On 06/01/2016 06:42, Dirk Laurie wrote:

> 2016-01-05 23:06 GMT+02:00 Lorenzo Donati <[hidden email]>:
>>
>>
>> On 05/01/2016 21:12, Dirk Laurie wrote:
>>>
>>> 2016-01-05 19:18 GMT+02:00 Lorenzo Donati <[hidden email]>:
>>>>
>>>>
>>>>
>>>> On 01/01/2016 15:30, Roberto Ierusalimschy wrote:

[snip]

>
> As usual with Lua bugs, the bug and its fix have been reported
> on http://www.lua.org/bugs.html. The description of the bug is:
>
>     Metatable may access its own deallocated field when
>     it has a self reference in __newindex.
>
> The question asked was:  Should this bug require a "fast-track"
> emergency bug-fix release of 5.3.3?
>
> Lorenzo has ably summarized the reasons why it should, most
> of which would be applicable to any bug. About the only one that
> is unusual is that the fault shows up as a segfault, not a stack
> overflow or some other error that can be trapped at the Lua level.
>
> There are also reasons why it would be unusual to put out an
> emergency bug-fix release of a new minor version, most of which
> would be applicable to any bug.
>
> 1. The bug might not manifest itself on all machines.
> 2. The fix may introduce a new bug.
> 3. The combination of 1 and 2 would imply that an existing
> working Lua version on that machine has needlessy been
> broken.
>

OK, fair enough.

> The Lua 5.3.1 bug "io.lines does not check maximum number
> of options" also causes a segfault on my machine. The code
> to trigger it is also nothing esoteric. Yet 5.3.2-rc1 was only released
> four months later the bug was reported.
>

Of course I didn't mean Lua Team should urge to put a hastily conceived
fix out of the door. I meant to ask whether this only bug was deemed so
severe to warrant the *starting of the release procedure* (with all the
due testing according to Lua Team's usual accuracy) without waiting some
other bug to appear or some predefined amount of time (if there is such
a thing in the Team's schedule).


> I don't think the self-referencing __newindex bug is any worse,
> certainly not emergency material.
>
> However, the word "patches" among the quick links on the Download
> page
>     http://www.lua.org/download.html
> is perhaps a little cryptic. It would be helpful, in the situations that
> Lorenzo has highlighted, to have a sentence explaining that bugs
> (if any) reported after the release was frozen can be found there.
>
>

Yep, I didn't notice until now. The link is marked "patches", but the
title of the page is "bugs". A little inconsistent. Maybe renaming the
link to "bugs" as well would be better and clearer (especially for
newbies which couldn't know what a "patch" is). If the authors deem the
link should convey the info "here are the patches" then renaming both
page and link to "bugs & patches" would be better IMO, although not as
short and neat.

The clarification sentence you mention could be put in some help popup
to appear as one hovers the mouse over the link.

Probably the best is to rename the link as "bugs" and put the mention of
patches and the clarification in a popup for that link or/and in the top
part of the bugs page.

Cheers!

-- Lorenzo