rstudio / httpuv

HTTP and WebSocket server package for R
Other
224 stars 84 forks source link

Fix use-after-free error #363

Closed jcheng5 closed 1 year ago

jcheng5 commented 1 year ago

This has come up on the CRAN checks for clang-ASAN and gcc-ASAN.

gcc-ASAN results

```* using log directory ‘/data/gannet/ripley/R/packages/tests-gcc-SAN/httpuv.Rcheck’ * using R Under development (unstable) (2023-05-08 r84402) * using platform: x86_64-pc-linux-gnu (64-bit) * R was compiled by gcc (GCC) 13.1.0 GNU Fortran (GCC) 13.1.0 * running under: Fedora Linux 36 (Workstation Edition) * using session charset: UTF-8 * using option ‘--no-stop-on-test-error’ * checking for file ‘httpuv/DESCRIPTION’ ... OK * checking extension type ... Package * this is package ‘httpuv’ version ‘1.6.10’ * package encoding: UTF-8 * checking package dependencies ... OK * checking if this is a source package ... OK * checking if there is a namespace ... OK * checking for hidden files and directories ... OK * checking for portable file names ... OK * checking whether package ‘httpuv’ can be installed ... [654s/261s] OK * used C compiler: ‘gcc (GCC) 12.2.1 20221121 (Red Hat 12.2.1-4)’ * used C++ compiler: ‘g++ (GCC) 13.1.0’ * checking package directory ... OK * checking whether the package can be loaded ... OK * checking whether the package can be loaded with stated dependencies ... OK * checking whether the package can be unloaded cleanly ... OK * checking whether the namespace can be loaded with stated dependencies ... OK * checking whether the namespace can be unloaded cleanly ... OK * checking loading without being on the library search path ... OK * checking compiled code ... OK * checking examples ... OK * checking tests ... ERROR Running ‘testthat.R’ Running the tests in ‘tests/testthat.R’ failed. Complete output: > library(testthat) > library(httpuv) > > test_check("httpuv") Using libcurl 8.0.1 with OpenSSL/3.0.8 ================================================================= ==2254896==ERROR: AddressSanitizer: heap-use-after-free on address 0x6130000c0960 at pc 0x7fd0f02db4c0 bp 0x7fd0ee2229c0 sp 0x7fd0ee2229b8 READ of size 8 at 0x6130000c0960 thread T2 #0 0x7fd0f02db4bf in uv__run_closing_handles src/unix/core.c:320 #1 0x7fd0f02db4bf in uv_run src/unix/core.c:399 #2 0x7fd0f0151358 in io_thread(void*) /data/gannet/ripley/R/packages/tests-gcc-SAN/httpuv/src/httpuv.cpp:136 #3 0x7fd10168cdcc in start_thread (/lib64/libc.so.6+0x8cdcc) #4 0x7fd10171262f in __GI___clone3 (/lib64/libc.so.6+0x11262f) 0x6130000c0960 is located 288 bytes inside of 360-byte region [0x6130000c0840,0x6130000c09a8) freed by thread T2 here: #0 0x7fd102cbc0a8 in operator delete(void*, unsigned long) (/lib64/libasan.so.8+0xbc0a8) #1 0x7fd0f00a5062 in WebSocketConnection::~WebSocketConnection() /data/gannet/ripley/R/packages/tests-gcc-SAN/httpuv/src/websockets.h:199 #2 0x7fd0f00a5062 in void auto_deleter_background(WebSocketConnection*) /data/gannet/ripley/R/packages/tests-gcc-SAN/httpuv/src/auto_deleter.h:45 #3 0x7fd0f009ecf5 in std::_Sp_counted_deleter, (__gnu_cxx::_Lock_policy)2>::_M_dispose() /usr/local/gcc13/include/c++/13.1.0/bits/shared_ptr_base.h:527 #4 0x7fd0f0096ee4 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() /usr/local/gcc13/include/c++/13.1.0/bits/shared_ptr_base.h:346 #5 0x7fd0f00c8f82 in std::__shared_ptr::~__shared_ptr() /usr/local/gcc13/include/c++/13.1.0/bits/shared_ptr_base.h:1524 #6 0x7fd0f00c8f82 in std::shared_ptr::~shared_ptr() /usr/local/gcc13/include/c++/13.1.0/bits/shared_ptr.h:175 #7 0x7fd0f00c8f82 in HttpRequest::_on_closed(uv_handle_s*) /data/gannet/ripley/R/packages/tests-gcc-SAN/httpuv/src/httprequest.cpp:678 #8 0x7fd0f02dac45 in uv__finish_close src/unix/core.c:307 #9 0x7fd0f02dac45 in uv__run_closing_handles src/unix/core.c:321 #10 0x7fd0f02dac45 in uv_run src/unix/core.c:399 #11 0x7fd0f0151358 in io_thread(void*) /data/gannet/ripley/R/packages/tests-gcc-SAN/httpuv/src/httpuv.cpp:136 #12 0x7fd10168cdcc in start_thread (/lib64/libc.so.6+0x8cdcc) previously allocated by thread T2 here: #0 0x7fd102cbb1a8 in operator new(unsigned long) (/lib64/libasan.so.8+0xbb1a8) #1 0x7fd0f00b579e in HttpRequest::_initializeSocket() /data/gannet/ripley/R/packages/tests-gcc-SAN/httpuv/src/httprequest.h:208 #2 0x7fd0f00b8bbf in createHttpRequest(uv_loop_s*, std::shared_ptr, std::shared_ptr, CallbackQueue*) /data/gannet/ripley/R/packages/tests-gcc-SAN/httpuv/src/httprequest.h:235 #3 0x7fd0f009cf51 in on_request(uv_stream_s*, int) /data/gannet/ripley/R/packages/tests-gcc-SAN/httpuv/src/http.cpp:35 #4 0x7fd0f031239c in uv__server_io src/unix/stream.c:570 #5 0x7fd0f0343784 in uv__io_poll src/unix/epoll.c:374 #6 0x7fd0f02da901 in uv_run src/unix/core.c:389 #7 0x7fd0f0151358 in io_thread(void*) /data/gannet/ripley/R/packages/tests-gcc-SAN/httpuv/src/httpuv.cpp:136 #8 0x7fd10168cdcc in start_thread (/lib64/libc.so.6+0x8cdcc) Thread T2 created by T0 here: #0 0x7fd102c4b3e6 in __interceptor_pthread_create (/lib64/libasan.so.8+0x4b3e6) #1 0x7fd0f031bc94 in uv_thread_create_ex src/unix/thread.c:259 #2 0x7fd0f031beff in uv_thread_create src/unix/thread.c:213 #3 0x7fd0f0152489 in ensure_io_thread() /data/gannet/ripley/R/packages/tests-gcc-SAN/httpuv/src/httpuv.cpp:164 #4 0x7fd0f0152b75 in makeTcpServer(std::__cxx11::basic_string, std::allocator > const&, int, Rcpp::Function_Impl, Rcpp::Function_Impl, Rcpp::Function_Impl, Rcpp::Function_Impl, Rcpp::Function_Impl, Rcpp::Function_Impl, Rcpp::Vector<19, Rcpp::PreserveStorage>, Rcpp::Vector<19, Rcpp::PreserveStorage>, bool) /data/gannet/ripley/R/packages/tests-gcc-SAN/httpuv/src/httpuv.cpp:274 #5 0x7fd0f0064020 in _httpuv_makeTcpServer /data/gannet/ripley/R/packages/tests-gcc-SAN/httpuv/src/RcppExports.cpp:54 #6 0x58b805 in R_doDotCall /data/gannet/ripley/R/svn/R-devel/src/main/dotcode.c:909 SUMMARY: AddressSanitizer: heap-use-after-free src/unix/core.c:320 in uv__run_closing_handles Shadow bytes around the buggy address: 0x0c26800100d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c26800100e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c26800100f0: fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa fa 0x0c2680010100: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd 0x0c2680010110: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd =>0x0c2680010120: fd fd fd fd fd fd fd fd fd fd fd fd[fd]fd fd fd 0x0c2680010130: fd fd fd fd fd fa fa fa fa fa fa fa fa fa fa fa 0x0c2680010140: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c2680010150: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c2680010160: fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa 0x0c2680010170: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==2254896==ABORTING * DONE Status: 1 ERROR ```

The was happening due to a mistake on my part; I had made _pingTimer an inline uv_timer_t in the WebSocketConnection class, and called uv_close(&_pingTimer) from the WebSocketConnection destructor. That doesn't work because uv_close() is asynchronous, and the handle needs to stay allocated until the close callback is invoked.

I fixed this by heap-allocating the _pingTimer and freeing it in a uv_close callback. I'm sure there's a nicer pattern for doing this (or I'm sure that we could come up with one), but this can at least be a starting point for discussion or a temporary fix.

jcheng5 commented 1 year ago

Oh I guess _pingTimer should be _pPingTimer now that it's a pointer?