Re: [PATCH 1/2] BUG/MINOR: lua: Fix default value for pattern in Socket.receive

[Tim Düsterhus]

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

[Willy Tarreau]

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

[Tim Düsterhus]

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

[Thierry Fournier]

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, >b);
> -- 
> 2.15.1
> 

[PATCH 1/2] BUG/MINOR: lua: Fix default value for pattern in Socket.receive

[Tim Duesterhus]

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, >b);
-- 
2.15.1