Yielding in Close Call through Vararg Return Changes Number of Returns

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Yielding in Close Call through Vararg Return Changes Number of Returns

Xmilia Hermit
Hi,

it seems that yielding in a close call when the variable is closed by a
vararg return can mess up the number of arguments returned since after
the yield the OP_RETURN is executed again, however, it changed L->top to
ci->top before the close call and so after the yield in the second
execution of OP_RETURN n = cast_int(L->top - ra); will result in a
different n.
The following script shows this problem (compare the output for BUG =
true vs false):

local BUG = true

local function test()
     local x <close> = setmetatable({}, {
         __close = function()
             if BUG then coroutine.yield() end
         end
     })
     if not BUG then coroutine.yield() end
     return print("Return")
end

local c = coroutine.wrap(test)
c()
print(c())

The following lines of code seem to be the cause:
https://github.com/lua/lua/blob/eadd8c7178c79c814ecca9652973a9b9dd4cc71b/lvm.c#L851 
(Repeat OP_RETURN instruction after yield)
https://github.com/lua/lua/blob/eadd8c7178c79c814ecca9652973a9b9dd4cc71b/lvm.c#L1674 
(Change top before yield but not reset)
https://github.com/lua/lua/blob/eadd8c7178c79c814ecca9652973a9b9dd4cc71b/lvm.c#L1670 
(Wrong calculation of n after yield)


Regards,
Xmilia
Reply | Threaded
Open this post in threaded view
|

Patch for luaossl for Lua 5.4.3 (warning for users of luaL_Buffer!)

Paul Ducklin
Luaossl's bindings to the OpenSSL cipher functions don't work under Lua 5.4.3. You need to make the changes shown below. 

Importantly, you can't rely on lua_gettop() to tell you the stack position of the caller's last argument after you have done a luaL_buffinit(), because buffinit now pushes a temporary value on the stack when you use it. (See the 5.4.2 -> 5.4.3 diffs for lauxlib.c.)

The manual has long warned about luaL_Buffer functions using the stack: "During its normal operation, a string buffer uses a variable number of stack slots. So, while using a buffer, you cannot assume that you know where the top of the stack is."

But until 5.4.3 the code in luaossl worked in practice even though it was wrong in theory. I was perplexed and even annoyed by this bug at first until it occurred to me to RTFM, after which I was annoyed at myself, because although I clearly remembered having read the above warning before, I did not remember the actual content of what I had read ;-)

---change this code (and anything like it you have)---
static int cipher_update(lua_State *L) {
        EVP_CIPHER_CTX *ctx = checksimple(L, 1, CIPHER_CLASS);
        luaL_Buffer B;

        luaL_buffinit(L, &B);

        if (!cipher_update_(L, ctx, &B, 2, lua_gettop(L)))
                goto sslerr;
[. . . .]


static int cipher_final(lua_State *L) {
        EVP_CIPHER_CTX *ctx = checksimple(L, 1, CIPHER_CLASS);
        luaL_Buffer B;
        size_t block;
        int out;

        luaL_buffinit(L, &B);

        if (!cipher_update_(L, ctx, &B, 2, lua_gettop(L)))
                goto sslerr;
[. . . .]
----end---

---to look like this instead---
static int cipher_update(lua_State *L) {
        EVP_CIPHER_CTX *ctx = checksimple(L, 1, CIPHER_CLASS);
        luaL_Buffer B;
        int lastarg = lua_gettop(L); /* buffinit changes stack top */

        luaL_buffinit(L, &B);

        if (!cipher_update_(L, ctx, &B, 2, lastarg))
                goto sslerr;
[. . . .]

static int cipher_final(lua_State *L) {
        EVP_CIPHER_CTX *ctx = checksimple(L, 1, CIPHER_CLASS);
        luaL_Buffer B;
        int lastarg = lua_gettop(L); /* buffinit changes stack top */
        size_t block;
        int out;

        luaL_buffinit(L, &B);

        if (!cipher_update_(L, ctx, &B, 2, lastarg))
                goto sslerr;
[. . . .]
---end---
Reply | Threaded
Open this post in threaded view
|

Re: Patch for luaossl for Lua 5.4.3 (warning for users of luaL_Buffer!)

Roberto Ierusalimschy
> But until 5.4.3 the code in luaossl worked in practice even though it was wrong in theory. [...]

Before 5.4.3, aux. buffers also used the stack, but only when their
content reached a minimum size (1K?).  So, there is a chance that your
code did not work in practice all the time, only when the practice did
not produce large enough strings.

From this point of view, the new implementation is more robust, as it
exposes this kind of bug.

-- Roberto
Reply | Threaded
Open this post in threaded view
|

Re: Patch for luaossl for Lua 5.4.3 (warning for users of luaL_Buffer!)

Paul Ducklin
>> But until 5.4.3 the code in luaossl worked in practice
>> even though it was wrong in theory. [...]

> Before 5.4.3, aux. buffers also used the stack, but only
> when their content reached a minimum size (1K?).  
> So, there is a chance that your code did not work in
> practice all the time

Errr, it's not "my" code - the note I submitted was for anyone who uses luaossl (by William Ahern and Daurnimator).

I know  Daurnimator must sometimes read this list because they replied to a previous comment about luaossl by asking me to submit a patch for some other issue via Github... but I don't have a Github account so it's here or nowhere. Hoping that they see this email.

As far as I can see, the relevant luaossl code was not affected in Lua 5.4.2 or earlier because the code only calls lua_gettop() once, immediately after calling luaL_buffinit() but before using the buffer, in order to work out the number of caller arguments.

So if I have read it correctly, the relevant functions, cipher:update() and cipher:final(), will always work correctly in Lua 5.4.2 and will always throw an error() in Lua 5.4.3.