httpc: fix a race in GC finalizers

`httpc` module has two GC-finalizers: the first one for a Lua http
client (C function `luaT_httpc_cleanup`) and the second one for a Lua
http chunked requests (C function `luaT_httpc_io_cleanup`) introduced in
commit 417c6cb ("httpc: introduce stream input/output interface").
In a C implementation HTTP requests depends on structures of HTTP client
and there is a problem with destroying Lua objects in `httpc` module -
these GC-finalizers are not synchronized. This could lead to at least
two problems:

There is a race with GC-finalization that leads to use-after-free errors
when HTTP client is collected before collecting HTTP request. In a
stacktrace the problem looks as below:

```
0x55ca7d47652e in crash_collect+256
0x55ca7d476f6a in crash_signal_cb+100
0x7fb876c42520 in __sigaction+80
0x55ca7d641e51 in curl_slist_free_all+35
0x55ca7d441498 in httpc_request_delete+45
0x55ca7d4653f1 in httpc_io_destroy+27
0x55ca7d4674bc in luaT_httpc_io_cleanup+36
0x55ca7d4e00c7 in lj_BC_FUNCC+70
0x55ca7d4f8364 in gc_call_finalizer+668
0x55ca7d4f8946 in gc_finalize+1387
0x55ca7d4f91e2 in gc_onestep+864
0x55ca7d4f9716 in lj_gc_fullgc+276
...
```

Lua object `http.client` could be GC-collected when chunked HTTP request
is alive. This will lead to an error "IllegalParams: io: request must be
io" because we call a method when Lua object is already a `nil`.

```lua
local url = 'https://bronevichok.ru/'
local c = require('http.client').new()
local r = c:get(url, {chunked = true})
c = nil
collectgarbage()
collectgarbage()
r:read(1) -- IllegalParams: io: request must be io
```

The patch introduces two functions: `httpc_env_finish` and
`curl_env_finish`, that prepares curl and httpc environments for
destruction. HTTP client's GC finalizer now calls `httpc_env_finish`
instead of `httpc_env_destroy`, this prevents from destroying memory
that could be in use by HTTP requests. Additionally `httpc_env_finish`
sets a flag `cleanup`. HTTP environment destroying is called when flag
`cleanup` is set and a there are no active HTTP requests. The main idea
of the patch is a synchronization of descructors for HTTP client and
HTTP chunked request. Unfortunately, GC will eventually collect HTTP
client object after calling its `__gc`. To prevent this we put a
reference to a userdata with HTTP client object to a `LUA_REGISTRYINDEX`
and remove this reference only when GC-finalizer for HTTP client is
called and there are no active HTTP chunked requests.

Fixes tarantool#9346
Fixes tarantool#9453

NO_DOC=bugfix

changelogs/unreleased/gh-9346-crash-on-GC-collection-of-http-objects.md

@@ -0,0 +1,3 @@
+## bugfix/http
+
+* Fixed a crash due to a race in GC finalizers (gh-9346).

changelogs/unreleased/gh-9453-http-client-is-gc-collected-when-chunked-request-is-alive.md

@@ -0,0 +1,4 @@
+## bugfix/http
+
+* Fixed behaviour of GC-finalizer of HTTP client Lua object when chunked HTTP
+  request is alive. (gh-9453).

src/curl.c

@@ -259,7 +259,7 @@ curl_env_create(struct curl_env *env, long max_conns, long max_total_conns)
 }
 
 void
-curl_env_destroy(struct curl_env *env)
+curl_env_finish(struct curl_env *env)
 {
 	assert(env);
 	if (env->multi) {
@@ -276,6 +276,15 @@ curl_env_destroy(struct curl_env *env)
 		}
 		curl_multi_cleanup(env->multi);
 	}
+}
+
+void
+curl_env_destroy(struct curl_env *env)
+{
+	assert(env);
+
+	assert(mempool_count(&env->sock_pool) == 0);
+	assert(mempool_used(&env->sock_pool) == 0);
 
 	mempool_destroy(&env->sock_pool);
 }

src/curl.h

@@ -111,6 +111,13 @@ curl_env_create(struct curl_env *env, long max_conns, long max_total_conns);
 void
 curl_env_destroy(struct curl_env *env);
 
+/**
+ * Finish HTTP client environment
+ * @param env pointer to an environment to finish
+ */
+void
+curl_env_finish(struct curl_env *env);
+
 /**
  * Initialize a new CURL request
  * @param curl_request request

src/httpc.c

@@ -140,6 +140,15 @@ httpc_env_create(struct httpc_env *env, int max_conns, int max_total_conns)
 	return curl_env_create(&env->curl_env, max_conns, max_total_conns);
 }
 
+void
+httpc_env_finish(struct httpc_env *ctx)
+{
+	assert(ctx);
+
+	curl_env_finish(&ctx->curl_env);
+	ctx->cleanup = 1;
+}
+
 void
 httpc_env_destroy(struct httpc_env *ctx)
 {
@@ -208,12 +217,18 @@ httpc_request_new(struct httpc_env *env, const char *method,
 
 	ibuf_create(&req->send, &cord()->slabc, 1);
 
+	++env->req_count;
+
 	return req;
 }
 
 void
 httpc_request_delete(struct httpc_request *req)
 {
+	struct httpc_env *env = req->env;
+	--env->req_count;
+	assert(env->req_count >= 0);
+
 	if (req->headers != NULL)
 		curl_slist_free_all(req->headers);
 
@@ -230,6 +245,11 @@ httpc_request_delete(struct httpc_request *req)
 	}
 
 	mempool_free(&req->env->req_pool, req);
+
+	if (env->req_count == 0 &&
+	    env->cleanup == 1) {
+		httpc_env_destroy(env);
+	}
 }
 
 int

src/httpc.h

@@ -60,12 +60,18 @@ struct httpc_stat {
  * HTTP Client Environment
  */
 struct httpc_env {
+	/** Number of a current active http requests. */
+	int req_count;
+	/** Flag that triggers cleanup. */
+	int cleanup;
 	/** Curl environment. */
 	struct curl_env curl_env;
 	/** Memory pool for requests */
 	struct mempool req_pool;
 	/** Statistics */
 	struct httpc_stat stat;
+	/** Lua reference. */
+	int lua_ref;
 };
 
 /**
@@ -78,6 +84,13 @@ struct httpc_env {
 int
 httpc_env_create(struct httpc_env *ctx, int max_conns, int max_total_conns);
 
+/**
+ * Finish HTTP client environment
+ * @param env pointer to a structure to finish
+ */
+void
+httpc_env_finish(struct httpc_env *env);
+
 /**
  * Destroy HTTP client environment
  * @param env pointer to a structure to destroy

src/lua/httpc.c

@@ -495,6 +495,9 @@ luaT_httpc_new(lua_State *L)
 	if (httpc_env_create(ctx, max_conns, max_total_conns) != 0)
 		return luaT_error(L);
 
+	lua_pushvalue(L, -1); /* Popped by luaL_ref(). */
+	ctx->lua_ref = luaL_ref(L, LUA_REGISTRYINDEX);
+
 	luaL_getmetatable(L, DRIVER_LUA_UDATA_NAME);
 	lua_setmetatable(L, -2);
 
@@ -504,7 +507,12 @@ luaT_httpc_new(lua_State *L)
 static int
 luaT_httpc_cleanup(lua_State *L)
 {
-	httpc_env_destroy(luaT_httpc_checkenv(L));
+	struct httpc_env *env = luaT_httpc_checkenv(L);
+	httpc_env_finish(env);
+	if (env->req_count == 0) {
+		httpc_env_destroy(env);
+		luaL_unref(L, LUA_REGISTRYINDEX, env->lua_ref);
+	}
 
 	/** remove all methods operating on ctx */
 	lua_newtable(L);
@@ -607,7 +615,11 @@ luaT_httpc_io_finish(lua_State *L)
 static int
 luaT_httpc_io_cleanup(lua_State *L)
 {
-	httpc_io_destroy(luaT_httpc_checkio(L));
+	struct httpc_io *io = luaT_httpc_checkio(L);
+	struct httpc_env *env = io->req->env;
+	httpc_io_destroy(io);
+	if (env->req_count == 0)
+		luaL_unref(L, LUA_REGISTRYINDEX, env->lua_ref);
 
 	/** remove all methods operating on io */
 	lua_newtable(L);

src/lua/httpc.lua

@@ -614,6 +614,9 @@ curl_mt = {
                         write = io_write,
                         finish = io_finish,
                     },
+                    _internal = {
+                        io = resp._internal.io,
+                    },
                 })
             end
         end,

test/app-luatest/http_client_test.lua

@@ -1375,3 +1375,57 @@ g.test_http_client_io_post_parts = function(cg)
     t.assert_equals(io.status, 200, 'io')
     t.assert_equals(io.reason, 'Ok', '200 - Ok')
 end
+
+-- Call __gc metamethod and disable it on a passed object.
+local function call_gc_finalizer_and_disable(obj)
+    local mt_orig = debug.getmetatable(obj)
+    local mt = table.deepcopy(mt_orig)
+    mt.__gc(obj)
+    mt.__gc = nil
+    debug.setmetatable(obj, mt)
+end
+
+-- GH-9453: GC has collected HTTP client object
+-- when HTTP IO object was alive.
+-- Symptom: A message "IllegalParams: io: request must be io"
+-- is happened on calling HTTP IO methods.
+g.test_http_client_gc_finalizer_gh_9453 = function(cg)
+    local url, opts = cg.url, table.deepcopy(cg.opts)
+    opts.chunked = true
+    local httpc_gh_9453 = require('http.client').new()
+
+    local http_io = httpc_gh_9453:get(url, opts)
+
+    -- GC collect HTTP client object.
+    call_gc_finalizer_and_disable(httpc_gh_9453.curl)
+    httpc_gh_9453 = nil -- luacheck: no unused
+    collectgarbage()
+    collectgarbage()
+
+    -- HTTP IO object is alive.
+    local ok, res = pcall(http_io.read, http_io, 4)
+    t.assert_equals(ok, true)
+    t.assert_equals(res, 'hell')
+
+    -- Teardown.
+    http_io = nil -- luacheck: no unused
+    collectgarbage()
+    collectgarbage()
+end
+
+g.test_gh_9346_httpc_io_cleanup = function(cg)
+    local url, opts = cg.url, table.deepcopy(cg.opts)
+    opts.chunked = true
+
+    local httpc_gh_9346 = require('http.client').new()
+    local http_io = httpc_gh_9346:get(url, opts) -- luacheck: no unused
+
+    call_gc_finalizer_and_disable(httpc_gh_9346.curl)
+    call_gc_finalizer_and_disable(http_io._internal.io)
+
+    -- Teardown.
+    httpc_gh_9346 = nil -- luacheck: no unused
+    http_io = nil -- luacheck: no unused
+    collectgarbage()
+    collectgarbage()
+end