Re: [PATCH 1/2] BUG/MINOR: lua: Fix default value for pattern in Socket.receive
Willy, Am 09.01.2018 um 20:07 schrieb Willy Tarreau: > Indeed, we try to perform backports by batches because it requires a > bit of concentration. Understood. > That's why it's important for the stable team > that the commits are properly marked for backports, as you did. So > thanks for that ;-) It pays to read the CONTRIBUTING file :-) Best regards Tim Düsterhus
Re: [PATCH 1/2] BUG/MINOR: lua: Fix default value for pattern in Socket.receive
On Tue, Jan 09, 2018 at 06:55:59PM +0100, Tim Düsterhus wrote: > Willy, I notice that you did not backport to earlier than 1.8, yet. Do > you usually do this shortly before release or did you forget? At least > the two MINOR ones should be backported to 1.6 also. Indeed, we try to perform backports by batches because it requires a bit of concentration. That's why it's important for the stable team that the commits are properly marked for backports, as you did. So thanks for that ;-) Willy
Re: [PATCH 1/2] BUG/MINOR: lua: Fix default value for pattern in Socket.receive
Hi Am 09.01.2018 um 15:24 schrieb Willy Tarreau: > On Mon, Jan 08, 2018 at 11:35:47AM +0100, Thierry Fournier wrote: >> Thanks for the patch. Good catch ! >> Willy, you can apply it. > > OK thanks for the review, all 4 patches applied now. > Thank you both. Willy, I notice that you did not backport to earlier than 1.8, yet. Do you usually do this shortly before release or did you forget? At least the two MINOR ones should be backported to 1.6 also. Best regards Tim Düsterhus
Re: [PATCH 1/2] BUG/MINOR: lua: Fix default value for pattern in Socket.receive
Hi guys, On Mon, Jan 08, 2018 at 11:35:47AM +0100, Thierry Fournier wrote: > Hi Tim, > > Thanks for the patch. Good catch ! > Willy, you can apply it. OK thanks for the review, all 4 patches applied now. Willy
Re: [PATCH 1/2] BUG/MINOR: lua: Fix default value for pattern in Socket.receive
Hi Tim, Thanks for the patch. Good catch ! Willy, you can apply it. Thierry > On 4 Jan 2018, at 19:32, Tim Duesterhus wrote: > > The default value of the pattern in `Socket.receive` is `*l` according > to the documentation and in the `socket.tcp.receive` method of Lua. > > The default value of `wanted` in `int hlua_socket_receive(struct lua_State *)` > reflects this requirement, but the function fails to ensure this > nonetheless: > > If no parameter is given the top of the Lua stack will have the index 1. > `lua_pushinteger(L, wanted);` then pushes the default value onto the stack > (with index 2). > The following `lua_replace(L, 2);` then pops the top index (2) and tries to > replace the index 2 with it. > I am not sure why exactly that happens (possibly, because one cannot replace > non-existent stack indicies), but this causes the stack index to be lost. > > `hlua_socket_receive_yield` then tries to read the stack index 2, to > determine what to read and get the value `0`, instead of the correct > HLSR_READ_LINE, thus taking the wrong branch. > > Fix this by ensuring that the top of the stack is not replaced by itself. > > This bug was introduced in commit 7e7ac32dad1e15c19152d37aaf9ea6b3f00a7226 > (which is the very first commit adding the Socket class to Lua). This > bugfix should be backported to every branch containing that commit: > - 1.6 > - 1.7 > - 1.8 > > A test case for this bug is as follows: > > The 'Test' response header will contain an HTTP status line with the > patch applied and will be empty without the patch applied. Replacing > the `sock:receive()` with `sock:receive("*l")` will cause the status > line to appear with and without the patch > > http.lua: > core.register_action("bug", { "http-req" }, function(txn) > local sock = core.tcp() > sock:settimeout(60) > sock:connect("127.0.0.1:80") > sock:send("GET / HTTP/1.0\r\n\r\n") > response = sock:receive() > sock:close() > txn:set_var("txn.foo", response) > end) > > haproxy.cfg (bits omitted for brevity): > global > lua-load /scratch/haproxy/http.lua > > frontend fe > bind 127.0.0.1:8080 > http-request lua.bug > http-response set-header Test %[var(txn.foo)] > > default_backend be > > backend be > server s 127.0.0.1:80 > --- > src/hlua.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/src/hlua.c b/src/hlua.c > index abd096d03..285d25589 100644 > --- a/src/hlua.c > +++ b/src/hlua.c > @@ -1869,7 +1869,10 @@ __LJMP static int hlua_socket_receive(struct lua_State > *L) > > /* Set pattern. */ > lua_pushinteger(L, wanted); > - lua_replace(L, 2); > + > + /* Check if we would replace the top by itself. */ > + if (lua_gettop(L) != 2) > + lua_replace(L, 2); > > /* init bufffer, and fiil it wih prefix. */ > luaL_buffinit(L, &socket->b); > -- > 2.15.1 >
[PATCH 1/2] BUG/MINOR: lua: Fix default value for pattern in Socket.receive
The default value of the pattern in `Socket.receive` is `*l` according to the documentation and in the `socket.tcp.receive` method of Lua. The default value of `wanted` in `int hlua_socket_receive(struct lua_State *)` reflects this requirement, but the function fails to ensure this nonetheless: If no parameter is given the top of the Lua stack will have the index 1. `lua_pushinteger(L, wanted);` then pushes the default value onto the stack (with index 2). The following `lua_replace(L, 2);` then pops the top index (2) and tries to replace the index 2 with it. I am not sure why exactly that happens (possibly, because one cannot replace non-existent stack indicies), but this causes the stack index to be lost. `hlua_socket_receive_yield` then tries to read the stack index 2, to determine what to read and get the value `0`, instead of the correct HLSR_READ_LINE, thus taking the wrong branch. Fix this by ensuring that the top of the stack is not replaced by itself. This bug was introduced in commit 7e7ac32dad1e15c19152d37aaf9ea6b3f00a7226 (which is the very first commit adding the Socket class to Lua). This bugfix should be backported to every branch containing that commit: - 1.6 - 1.7 - 1.8 A test case for this bug is as follows: The 'Test' response header will contain an HTTP status line with the patch applied and will be empty without the patch applied. Replacing the `sock:receive()` with `sock:receive("*l")` will cause the status line to appear with and without the patch http.lua: core.register_action("bug", { "http-req" }, function(txn) local sock = core.tcp() sock:settimeout(60) sock:connect("127.0.0.1:80") sock:send("GET / HTTP/1.0\r\n\r\n") response = sock:receive() sock:close() txn:set_var("txn.foo", response) end) haproxy.cfg (bits omitted for brevity): global lua-load /scratch/haproxy/http.lua frontend fe bind 127.0.0.1:8080 http-request lua.bug http-response set-header Test %[var(txn.foo)] default_backend be backend be server s 127.0.0.1:80 --- src/hlua.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/hlua.c b/src/hlua.c index abd096d03..285d25589 100644 --- a/src/hlua.c +++ b/src/hlua.c @@ -1869,7 +1869,10 @@ __LJMP static int hlua_socket_receive(struct lua_State *L) /* Set pattern. */ lua_pushinteger(L, wanted); - lua_replace(L, 2); + + /* Check if we would replace the top by itself. */ + if (lua_gettop(L) != 2) + lua_replace(L, 2); /* init bufffer, and fiil it wih prefix. */ luaL_buffinit(L, &socket->b); -- 2.15.1