Commits
- Commit:
1e7a1825795f51978307cac48ad686a04904c3d7
- From:
- Sergey Bronnikov <sergeyb@tarantool.org>
- Date:
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 417c6cb7b0c6 ("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 destructors for HTTP client and
HTTP chunked requests. Unfortunately, GC will eventually collect HTTP
client object after calling its `__gc`. To prevent this we put a
reference to a Curl's userdata in Lua objects with HTTP chunked requests
and HTTP default client.
Fixes #9346
Fixes #9453
NO_DOC=bugfix
- Commit:
b683490c9408a4c1a8e3e0cbf48214f60a7de867
- From:
- Sergey Bronnikov <sergeyb@tarantool.org>
- Date:
httpc: fix a crash triggered by gc
Bump curl version to 8.4.0 triggers a crash in Tarantool due to commit
"h2: testcase and fix for pausing h2 streams" [1]. The original
reproducer involves etcd and an etcd-client Lua module, running
etcd-client tests as a part of Tarantool integration testing is planned
to do in scope of [1].
However, the problem could be reproduced with a Lua code below:
```
local url = 'https://google.com/'
local c = require('http.client').new()
r1 = c:get(url, {chunked = true})
r1:read(1)
r2 = c:get(url, {chunked = true})
r2:read(1)
r3 = c:get(url, {chunked = true})
r3:read(1)
r4 = c:get(url, {chunked = true})
r4:read(1)
c = nil
collectgarbage()
collectgarbage()
r1:read(1)
r2:read(1)
r3:read(1)
r4:read(1)
collectgarbage()
collectgarbage()
```
According to Curl documentation, `curl_multi_cleanup` [1] must be called
before any easy handles are cleaned up. The patch adds a cleanup of easy
handles on running `curl_env_destroy`, right before calling
`curl_multi_cleanup`. The patch uses a function 'curl_multi_get_handles'
that returns all added easy handles introduced in Curl 8.4.0. Therefore
bump to 8.4.0 is required.
1. https://github.com/curl/curl/commit/6b9a591bf7d82031f463373706d7de1cba0adee6
2. https://curl.se/libcurl/c/curl_multi_cleanup.html
Fixes #9283
1. https://github.com/tarantool/tarantool/issues/9093
NO_DOC=bugfix
NO_TEST=no simple reproducer, covered by tests in etcd-client
- Commit:
345401063a6720e61f93f38a24732478f6628c2c
- From:
- Sergey Bronnikov <sergeyb@tarantool.org>
- Date:
httpc: prefer curl headers in submodule by default
FreeBSD instances in Tarantool CI have installed libcurl package (as a
dependency of Zabbix monitoring agent). Curl 8.4.0 introduces a new
function `curl_multi_get_handles` that is used in the following commit
in `src/curl.c`, but libcurl system package has no such symbol in
headers. On building on FreeBSD in Tarantool CI C compiler produces a
warning about implicit declaration of function, because it looks at
system headers by default and due to enabled CMake option
`-DENABLE_WERROR=ON` building has failed:
```
[ 63%] Building C object src/CMakeFiles/server.dir/title.c.o
/.cache/act/55d136250dd94303/hostexecutor/src/curl.c:266:17: error: implicit declaration of function 'curl_multi_get_handles' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
CURL **list = curl_multi_get_handles(env->multi);
^
/.cache/act/55d136250dd94303/hostexecutor/src/curl.c:266:17: note: did you mean 'curl_multi_add_handle'?
/usr/local/include/curl/multi.h:140:23: note: 'curl_multi_add_handle' declared here
CURL_EXTERN CURLMcode curl_multi_add_handle(CURLM *multi_handle,
^
/.cache/act/55d136250dd94303/hostexecutor/src/curl.c:266:10: error: incompatible integer to pointer conversion initializing 'CURL **' (aka 'void **') with an expression of type 'int' [-Werror,-Wint-conversion]
CURL **list = curl_multi_get_handles(env->multi);
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
```
The patch fixes that by reordering headers passed to compiler, see [1].
1. https://cmake.org/cmake/help/latest/command/include_directories.html
Needed for #9283
NO_CHANGELOG=build
NO_DOC=build
NO_TEST=build
- Commit:
9b7b739ff7ee78fa0de3dbb38015d0d11af0370f
- From:
- Sergey Bronnikov <sergeyb@tarantool.org>
- Date:
third_party: update libcurl from 8.3.0 to 8.4.0
The patch updates curl module to the version 8.4.0 [1] that brings a
number of functional fixes and security fix of SOCKS5 heap buffer
overflow (CVE-2023-38545), see description in [2] and commit
fb4415d8aee6 ("socks: return error if hostname too long for remote
resolve") in [3].
1. https://curl.se/changes.html#8_4_0
2. https://curl.se/docs/CVE-2023-38545.html
3. https://github.com/curl/curl/commit/fb4415d8aee6c1045be932a34fe6107c2f5ed147
NO_DOC=libcurl submodule bump
NO_TEST=libcurl submodule bump
- Commit:
aca863907e1c5bdbd108f4be17583083d2779e0a
- From:
- Sergey Bronnikov <sergeyb@tarantool.org>
- Date:
tests: suppress message 'Broken pipe exception handling'
Message below is printed every time on shutdown `httpd.py` when
`test/app-luatest/http_client_test.lua` is running by luatest without
capturing stdout:
```
BrokenPipeError: [Errno 32] Broken pipe exception handling
```
The patch suppress this exception by adding a handler for a signal
`SIGPIPE`.
NO_CHANGELOG=testing
NO_DOC=testing
NO_TEST=testing