lua 5.3.2 introduced a bug related to coroutines

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

lua 5.3.2 introduced a bug related to coroutines

Nagaev Boris
$ cat bug.lua
local function yieldFirst(it)
    return coroutine.wrap(function()
        coroutine.yield(it())
    end)
end

local text = "1 2 3"
local it = text:gmatch("(%d+)")
local head = yieldFirst(it)
local one = head()
print(one)
assert(one == '1')

$ ./lua-5.3.1/src/lua bug.lua
1
$ ./lua-5.3.2/src/lua bug.lua
function: 0x243f820
./lua-5.3.2/src/lua: bug.lua:12: assertion failed!
stack traceback:
        [C]: in function 'assert'
        bug.lua:12: in main chunk
        [C]: in ?

--


Best regards,
Boris Nagaev

Reply | Threaded
Open this post in threaded view
|

Re: lua 5.3.2 introduced a bug related to coroutines

Soni "They/Them" L.


On 17/03/16 08:07 PM, Nagaev Boris wrote:

> $ cat bug.lua
> local function yieldFirst(it)
>      return coroutine.wrap(function()
>          coroutine.yield(it())
>      end)
> end
>
> local text = "1 2 3"
> local it = text:gmatch("(%d+)")
> local head = yieldFirst(it)
> local one = head()
> print(one)
> assert(one == '1')
>
> $ ./lua-5.3.1/src/lua bug.lua
> 1
> $ ./lua-5.3.2/src/lua bug.lua
> function: 0x243f820
> ./lua-5.3.2/src/lua: bug.lua:12: assertion failed!
> stack traceback:
>          [C]: in function 'assert'
>          bug.lua:12: in main chunk
>          [C]: in ?
>
It's even worse than it looks...

--bug.lua--
local function yieldFirst(it)
     return coroutine.wrap(function()
         local v = it()
         coroutine.yield(v)
         print(v) -- ???
     end)
end

local text = "1 2 3"
local it = text:gmatch("(%d+)")
local head = yieldFirst(it)
local one = head()
print(one)
head() -- ???
assert(one == '1')
--end--

$ lua bug.lua
function: 0x11c9030
function: 0x11c9030
lua: bug.lua:15: assertion failed!
stack traceback:
     [C]: in function 'assert'
     bug.lua:15: in main chunk
     [C]: in ?

Either something really bad is happening (because it's affecting locals,
not just the yielded values), or string iterators break across coroutines.

--
Disclaimer: these emails may be made public at any given time, with or without reason. If you don't agree with this, DO NOT REPLY.


Reply | Threaded
Open this post in threaded view
|

Re: lua 5.3.2 introduced a bug related to coroutines

William Ahern
In reply to this post by Nagaev Boris
On Fri, Mar 18, 2016 at 02:07:11AM +0300, Nagaev Boris wrote:

> $ cat bug.lua
> local function yieldFirst(it)
>     return coroutine.wrap(function()
>         coroutine.yield(it())
>     end)
> end
>
> local text = "1 2 3"
> local it = text:gmatch("(%d+)")
> local head = yieldFirst(it)
> local one = head()
> print(one)
> assert(one == '1')
>
> $ ./lua-5.3.1/src/lua bug.lua
> 1
> $ ./lua-5.3.2/src/lua bug.lua
> function: 0x243f820
> ./lua-5.3.2/src/lua: bug.lua:12: assertion failed!
> stack traceback:
>         [C]: in function 'assert'
>         bug.lua:12: in main chunk
>         [C]: in ?
>

My first guess is that lstrlib.c:gmatch is storing the lua_State object (see
prepstate) used when creating the closure. When the closure is invoked, it
uses this original lua_State instead of the current lua_State invoking the
closure. But the original lua_State is busy resuming the new lua_State.

The immediate solution might be to add

        gm->ms.L = L

to strlib.c:gmatch_aux. I haven't tested it. It might result in wrong error
messages? Maybe not. I don't know why lua_State is being stored in
MatchState except to reduce the number of arguments needed to invoke all the
other routines in strlib.c.


Reply | Threaded
Open this post in threaded view
|

Re: lua 5.3.2 introduced a bug related to coroutines

William Ahern
On Thu, Mar 17, 2016 at 06:07:51PM -0700, William Ahern wrote:

> On Fri, Mar 18, 2016 at 02:07:11AM +0300, Nagaev Boris wrote:
> > $ cat bug.lua
> > local function yieldFirst(it)
> >     return coroutine.wrap(function()
> >         coroutine.yield(it())
> >     end)
> > end
> >
> > local text = "1 2 3"
> > local it = text:gmatch("(%d+)")
> > local head = yieldFirst(it)
> > local one = head()
> > print(one)
> > assert(one == '1')
> >
> > $ ./lua-5.3.1/src/lua bug.lua
> > 1
> > $ ./lua-5.3.2/src/lua bug.lua
> > function: 0x243f820
> > ./lua-5.3.2/src/lua: bug.lua:12: assertion failed!
> > stack traceback:
> >         [C]: in function 'assert'
> >         bug.lua:12: in main chunk
> >         [C]: in ?
> >
>
> My first guess is that lstrlib.c:gmatch is storing the lua_State object (see
> prepstate) used when creating the closure. When the closure is invoked, it
> uses this original lua_State instead of the current lua_State invoking the
> closure. But the original lua_State is busy resuming the new lua_State.
>
> The immediate solution might be to add
>
> gm->ms.L = L
>

This is probably the correct solution. In Lua 5.2 the initialization of
MatchState happens every time gmatch_aux (the closure) is invoked. It looks
like for 5.3 this was refactored so that MatchState is only initialized
once. But what was forgotten in the process is that lua_State might change
when the closure is invoked.


Reply | Threaded
Open this post in threaded view
|

Re: lua 5.3.2 introduced a bug related to coroutines

Roberto Ierusalimschy
In reply to this post by William Ahern
> My first guess is that lstrlib.c:gmatch is storing the lua_State object (see
> prepstate) used when creating the closure. When the closure is invoked, it
> uses this original lua_State instead of the current lua_State invoking the
> closure. But the original lua_State is busy resuming the new lua_State.
>
> The immediate solution might be to add
>
> gm->ms.L = L

Seems right.


> to strlib.c:gmatch_aux. I haven't tested it. It might result in wrong error
> messages? Maybe not. I don't know why lua_State is being stored in
> MatchState except to reduce the number of arguments needed to invoke all the
> other routines in strlib.c.

That was the whole point of the original 'MatchState': to reduce the
number of arguments among the several routines in the regex engine.  It
was then hijacked by 'gmatch', which created this bug.

-- Roberto

Reply | Threaded
Open this post in threaded view
|

Re: lua 5.3.2 introduced a bug related to coroutines

Marc Balmer
In reply to this post by William Ahern

> Am 18.03.2016 um 02:07 schrieb William Ahern <[hidden email]>:
>
> On Fri, Mar 18, 2016 at 02:07:11AM +0300, Nagaev Boris wrote:
>> $ cat bug.lua
>> local function yieldFirst(it)
>>    return coroutine.wrap(function()
>>        coroutine.yield(it())
>>    end)
>> end
>>
>> local text = "1 2 3"
>> local it = text:gmatch("(%d+)")
>> local head = yieldFirst(it)
>> local one = head()
>> print(one)
>> assert(one == '1')
>>
>> $ ./lua-5.3.1/src/lua bug.lua
>> 1
>> $ ./lua-5.3.2/src/lua bug.lua
>> function: 0x243f820
>> ./lua-5.3.2/src/lua: bug.lua:12: assertion failed!
>> stack traceback:
>>        [C]: in function 'assert'
>>        bug.lua:12: in main chunk
>>        [C]: in ?
>>
>
> My first guess is that lstrlib.c:gmatch is storing the lua_State object (see
> prepstate) used when creating the closure. When the closure is invoked, it
> uses this original lua_State instead of the current lua_State invoking the
> closure. But the original lua_State is busy resuming the new lua_State.
>
> The immediate solution might be to add
>
> gm->ms.L = L

Where would one add that, right before

      return push_captures(&gm->ms, src, e);

?

>


> to strlib.c:gmatch_aux. I haven't tested it. It might result in wrong error
> messages? Maybe not. I don't know why lua_State is being stored in
> MatchState except to reduce the number of arguments needed to invoke all the
> other routines in strlib.c.
>
>


Reply | Threaded
Open this post in threaded view
|

Re: lua 5.3.2 introduced a bug related to coroutines

Tony Papadimitriou
In reply to this post by Roberto Ierusalimschy
It there going to be an official patch for this one?

-----Original Message-----
From: Roberto Ierusalimschy

> The immediate solution might be to add
>
> gm->ms.L = L

Seems right.

> to strlib.c:gmatch_aux. I haven't tested it. It might result in wrong
> error

That was the whole point of the original 'MatchState': to reduce the
number of arguments among the several routines in the regex engine.  It
was then hijacked by 'gmatch', which created this bug.

-- Roberto


Reply | Threaded
Open this post in threaded view
|

Re: lua 5.3.2 introduced a bug related to coroutines

Luiz Henrique de Figueiredo
> It there going to be an official patch for this one?

Yes. See http://www.lua.org/bugs.html#5.3.2-3 .