bugfix: wrong arguments of setkeepalive() result in the compromise of data integrity.

Author: zhuizhuhaomeng

src/ngx_stream_lua_socket_tcp.c

@@ -5250,6 +5250,34 @@ ngx_stream_lua_socket_tcp_setkeepalive(lua_State *L)
 
  luaL_checktype(L, 1, LUA_TTABLE);
 
+ r = ngx_stream_lua_get_req(L);
+ if (r == NULL) {
+ return luaL_error(L, "no request found");
+ }
+
+ llcf = ngx_stream_lua_get_module_loc_conf(r, ngx_stream_lua_module);
+
+ /* luaL_checkinteger will throw error if the argument is not a number.
+ * e.g.: bad argument \#2 to '?' (number expected, got string)
+ *
+ * We should check the argument in advance; otherwise,
+ * throwing an exception in the middle can compromise data integrity.
+ * e.g.: set pc->connection to NULL without following cleanup.
+ */
+ if (n >= 2 && !lua_isnil(L, 2)) {
+ timeout = (ngx_msec_t) luaL_checkinteger(L, 2);
+
+ } else {
+ timeout = llcf->keepalive_timeout;
+ }
+
+ if (n >= 3 && !lua_isnil(L, 3)) {
+ pool_size = luaL_checkinteger(L, 3);
+
+ } else {
+ pool_size = llcf->pool_size;
+ }
+
  lua_rawgeti(L, 1, SOCKET_CTX_INDEX);
  u = lua_touserdata(L, -1);
  lua_pop(L, 1);
@@ -5271,11 +5299,6 @@ ngx_stream_lua_socket_tcp_setkeepalive(lua_State *L)
  return 2;
  }
 
- r = ngx_stream_lua_get_req(L);
- if (r == NULL) {
- return luaL_error(L, "no request found");
- }
-
  if (u->request != r) {
  return luaL_error(L, "bad request");
  }
@@ -5349,18 +5372,9 @@ ngx_stream_lua_socket_tcp_setkeepalive(lua_State *L)
 
  /* stack: obj timeout? size? pools cache_key */
 
- llcf = ngx_stream_lua_get_module_loc_conf(r, ngx_stream_lua_module);
-
  if (spool == NULL) {
  /* create a new socket pool for the current peer key */
 
- if (n >= 3 && !lua_isnil(L, 3)) {
- pool_size = luaL_checkinteger(L, 3);
-
- } else {
- pool_size = llcf->pool_size;
- }
-
  if (pool_size <= 0) {
  msg = lua_pushfstring(L, "bad \"pool_size\" option value: %i",
  pool_size);
@@ -5425,13 +5439,6 @@ ngx_stream_lua_socket_tcp_setkeepalive(lua_State *L)
  ngx_del_timer(c->write);
  }
 
- if (n >= 2 && !lua_isnil(L, 2)) {
- timeout = (ngx_msec_t) luaL_checkinteger(L, 2);
-
- } else {
- timeout = llcf->keepalive_timeout;
- }
-
 #if (NGX_DEBUG)
  if (timeout == 0) {
  ngx_log_debug0(NGX_LOG_DEBUG_STREAM, r->connection->log, 0,

t/068-socket-keepalive.t

@@ -2354,3 +2354,141 @@ ok
 lua tcp socket abort queueing
 --- no_error_log
 [error]
+
+
+
+=== TEST 53: wrong first argument of setkeepalive()
+--- config
+ location /foo {
+ server_tokens off;
+ keepalive_timeout 60s;
+ echo foo;
+ }
+--- stream_server_config
+ content_by_lua_block {
+ local sock = ngx.socket.tcp()
+ sock:settimeouts(1000, 1000, 1000)
+
+ local ok, err = sock:connect("127.0.0.1", $TEST_NGINX_SERVER_PORT)
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+
+ ngx.say("connected: ", ok)
+
+ local req = "GET /foo HTTP/1.1\r\nHost: localhost\r\nConnection: keepalive\r\n\r\n"
+
+ local bytes, err = sock:send(req)
+ if not bytes then
+ ngx.say("failed to send request: ", err)
+ return
+ end
+
+ ngx.say("request sent: ", bytes)
+
+ local reader = sock:receiveuntil("\r\n0\r\n\r\n")
+ local data, err = reader()
+ if not data then
+ ngx.say("failed to receive response body: ", err)
+ return
+ end
+
+ ngx.say("received response of ", #data, " bytes")
+
+ local ok, err = sock:setkeepalive()
+ if not ok then
+ ngx.say("failed to set reusable: ", err)
+ return
+ end
+
+ ok, err = sock:connect("127.0.0.1", $TEST_NGINX_SERVER_PORT)
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+
+ local ok, err = sock:setkeepalive("not a number")
+ if not ok then
+ ngx.say("failed to set reusable: ", err)
+ return
+ end
+ }
+--- stream_response
+connected: 1
+request sent: 61
+received response of 156 bytes
+--- error_log eval
+qr/\Qbad argument #1 to 'setkeepalive' (number expected, got string)\E/
+--- no_error_log 
+[crit]
+--- timeout: 4
+
+
+
+=== TEST 54: wrong second argument of setkeepalive()
+--- config
+ location /foo {
+ server_tokens off;
+ keepalive_timeout 60s;
+ echo foo;
+ }
+--- stream_server_config
+ content_by_lua_block {
+ local sock = ngx.socket.tcp()
+ sock:settimeouts(1000, 1000, 1000)
+
+ local ok, err = sock:connect("127.0.0.1", $TEST_NGINX_SERVER_PORT)
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+
+ ngx.say("connected: ", ok)
+
+ local req = "GET /foo HTTP/1.1\r\nHost: localhost\r\nConnection: keepalive\r\n\r\n"
+
+ local bytes, err = sock:send(req)
+ if not bytes then
+ ngx.say("failed to send request: ", err)
+ return
+ end
+
+ ngx.say("request sent: ", bytes)
+
+ local reader = sock:receiveuntil("\r\n0\r\n\r\n")
+ local data, err = reader()
+ if not data then
+ ngx.say("failed to receive response body: ", err)
+ return
+ end
+
+ ngx.say("received response of ", #data, " bytes")
+
+ local ok, err = sock:setkeepalive()
+ if not ok then
+ ngx.say("failed to set reusable: ", err)
+ return
+ end
+
+ ok, err = sock:connect("127.0.0.1", $TEST_NGINX_SERVER_PORT)
+ if not ok then
+ ngx.say("failed to connect: ", err)
+ return
+ end
+
+ local ok, err = sock:setkeepalive(10, "not a number")
+ if not ok then
+ ngx.say("failed to set reusable: ", err)
+ return
+ end
+ }
+--- stream_response
+connected: 1
+request sent: 61
+received response of 156 bytes
+--- error_log eval
+qr/\Qbad argument #2 to 'setkeepalive' (number expected, got string)\E/
+--- no_error_log 
+[crit]
+--- timeout: 4