Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assertion failure in bio_cf_out_write if CURLOPT_HEADERFUNCTION callback gets CURLINFO_TLS_SSL_PTR #10336

Closed
fujii opened this issue Jan 23, 2023 · 9 comments

Comments

@fujii
Copy link

fujii commented Jan 23, 2023

I'm using libcurl 7.87.0 with libressl 3.7.0 for Windows WebKit.
After upgrading libcurl to 7.87.0 from 7.86, WebKit browser is crashing by loading https pages with a proxy.

With the debug libcurl.dll, I'm observing the following assertion failure.

Assertion failed!

Program: ...bkit\gb\WebKitLibraries\wincairo\bin64\libcurl.dll
File: D:\work\WinCairoReq\g\buildtrees\curl\src\7...\openssl.c
Line: 709

Expression: data

Callstack:

ucrtbased.dll!common_assert_to_message_box<wchar_t>(const wchar_t * const expression, const wchar_t * const file_name, const unsigned int line_number, void * const return_address) Line 388	C++
ucrtbased.dll!common_assert<wchar_t>(const wchar_t * const expression, const wchar_t * const file_name, const unsigned int line_number, void * const return_address) Line 424	C++
ucrtbased.dll!_wassert(const wchar_t * expression, const wchar_t * file_name, unsigned int line_number) Line 444	C++
libcurl.dll!bio_cf_out_write(bio_st * bio, const char * buf, int blen) Line 709	C
crypto-50.dll!BIO_write(bio_st * b, const void * in, int inl) Line 412	C
ssl-53.dll!tls13_legacy_wire_write(ssl_st * ssl, const unsigned char * buf, unsigned __int64 len) Line 75	C
ssl-53.dll!tls13_legacy_wire_write_cb(const void * buf, unsigned __int64 n, void * arg) Line 97	C
ssl-53.dll!tls13_record_send(tls13_record * rec, __int64(*)(const void *, unsigned __int64, void *) wire_write, void * wire_arg) Line 178	C
ssl-53.dll!tls13_record_layer_write_record(tls13_record_layer * rl, unsigned char content_type, const unsigned char * content, unsigned __int64 content_len) Line 1082	C
ssl-53.dll!tls13_record_layer_write_chunk(tls13_record_layer * rl, unsigned char content_type, const unsigned char * buf, unsigned __int64 n) Line 1101	C
ssl-53.dll!tls13_record_layer_write(tls13_record_layer * rl, unsigned char content_type, const unsigned char * buf, unsigned __int64 n) Line 1116	C
ssl-53.dll!tls13_write_handshake_data(tls13_record_layer * rl, const unsigned char * buf, unsigned __int64 n) Line 1164	C
ssl-53.dll!tls13_handshake_msg_send(tls13_handshake_msg * msg, tls13_record_layer * rl) Line 180	C
ssl-53.dll!tls13_handshake_send_action(tls13_ctx * ctx, const tls13_handshake_action * action) Line 470	C
ssl-53.dll!tls13_handshake_perform(tls13_ctx * ctx) Line 410	C
ssl-53.dll!tls13_client_connect(tls13_ctx * ctx) Line 85	C
ssl-53.dll!tls13_legacy_connect(ssl_st * ssl) Line 462	C
ssl-53.dll!SSL_connect(ssl_st * s) Line 993	C
libcurl.dll!ossl_connect_step2(Curl_cfilter * cf, Curl_easy * data) Line 3901	C
libcurl.dll!ossl_connect_common(Curl_cfilter * cf, Curl_easy * data, bool nonblocking, bool * done) Line 4441	C
libcurl.dll!ossl_connect_nonblocking(Curl_cfilter * cf, Curl_easy * data, bool * done) Line 4475	C
libcurl.dll!ssl_connect_nonblocking(Curl_cfilter * cf, Curl_easy * data, bool * done) Line 358	C
libcurl.dll!ssl_cf_connect(Curl_cfilter * cf, Curl_easy * data, bool blocking, bool * done) Line 1526	C
libcurl.dll!Curl_conn_connect(Curl_easy * data, int sockindex, bool blocking, bool * done) Line 367	C
libcurl.dll!multi_runsingle(Curl_multi * multi, curltime * nowp, Curl_easy * data) Line 2070	C
libcurl.dll!curl_multi_perform(Curl_multi * multi, int * running_handles) Line 2690	C
WebCore.dll!WebCore::CurlMultiHandle::perform(int & runningHandles) Line 281	C++
WebCore.dll!WebCore::CurlRequestScheduler::workerThread() Line 176	C++
WebCore.dll!WebCore::CurlRequestScheduler::startOrWakeUpThread::__l2::<lambda_1>::operator()() Line 99	C++
WebCore.dll!WTF::Detail::CallableWrapper<`WebCore::CurlRequestScheduler::startOrWakeUpThread'::`2'::<lambda_1>,void>::call() Line 53	C++
WTF.dll!WTF::Function<void __cdecl(void)>::operator()() Line 83	C++
WTF.dll!WTF::Thread::entryPoint(WTF::Thread::NewThreadContext * newThreadContext) Line 250	C++
WTF.dll!WTF::wtfThreadEntryPoint(void * data) Line 151	C++
ucrtbase.dll!thread_start<unsigned int (__cdecl*)(void *),1>()	Unknown
kernel32.dll!00007ff986977614()	Unknown
ntdll.dll!00007ff9870626a1()	Unknown

This assertion was added by this change.
55807e6
#9962

data is cleared because CURLOPT_HEADERFUNCTION callback calls curl_easy_getinfo with CURLINFO_TLS_SSL_PTR.

callstack of cf_ctx_set_data:

libcurl.dll!cf_ctx_set_data(Curl_cfilter * cf, Curl_easy * data) Line 325	C
libcurl.dll!Curl_ssl_get_internals(Curl_easy * data, int sockindex, CURLINFO info, int n) Line 1725	C
libcurl.dll!getinfo_slist(Curl_easy * data, CURLINFO info, curl_slist * * param_slistp) Line 536	C
libcurl.dll!Curl_getinfo(Curl_easy * data, CURLINFO info, ...) Line 604	C
libcurl.dll!curl_easy_getinfo(Curl_easy * data, CURLINFO info, ...) Line 816	C
WebCore.dll!WebCore::CurlHandle::sslConnection() Line 802	C++
WebCore.dll!WebCore::CurlHandle::certificateInfo() Line 944	C++
WebCore.dll!WebCore::CurlRequest::didReceiveHeader(WTF::String && header) Line 384	C++
WebCore.dll!WebCore::CurlRequest::didReceiveHeaderCallback(char * ptr, unsigned __int64 blockSize, unsigned __int64 numberOfBlocks, void * userData) Line 824	C++
libcurl.dll!chop_write(Curl_easy * data, int type, char * optr, unsigned __int64 olen) Line 633	C
libcurl.dll!Curl_client_write(Curl_easy * data, int type, char * ptr, unsigned __int64 len) Line 678	C
libcurl.dll!recv_CONNECT_resp(Curl_easy * data, connectdata * conn, tunnel_state * ts, bool * done) Line 583	C
libcurl.dll!CONNECT(Curl_cfilter * cf, Curl_easy * data, tunnel_state * ts) Line 991	C
libcurl.dll!http_proxy_cf_connect(Curl_cfilter * cf, Curl_easy * data, bool blocking, bool * done) Line 1084	C
libcurl.dll!ssl_cf_connect(Curl_cfilter * cf, Curl_easy * data, bool blocking, bool * done) Line 1512	C
libcurl.dll!Curl_conn_connect(Curl_easy * data, int sockindex, bool blocking, bool * done) Line 367	C
libcurl.dll!multi_runsingle(Curl_multi * multi, curltime * nowp, Curl_easy * data) Line 2070	C
libcurl.dll!curl_multi_perform(Curl_multi * multi, int * running_handles) Line 2690	C
WebCore.dll!WebCore::CurlMultiHandle::perform(int & runningHandles) Line 281	C++
WebCore.dll!WebCore::CurlRequestScheduler::workerThread() Line 176	C++
WebCore.dll!WebCore::CurlRequestScheduler::startOrWakeUpThread::__l2::<lambda_1>::operator()() Line 99	C++
WebCore.dll!WTF::Detail::CallableWrapper<`WebCore::CurlRequestScheduler::startOrWakeUpThread'::`2'::<lambda_1>,void>::call() Line 53	C++
WTF.dll!WTF::Function<void __cdecl(void)>::operator()() Line 83	C++
WTF.dll!WTF::Thread::entryPoint(WTF::Thread::NewThreadContext * newThreadContext) Line 250	C++
WTF.dll!WTF::wtfThreadEntryPoint(void * data) Line 151	C++
ucrtbase.dll!thread_start<unsigned int (__cdecl*)(void *),1>()	Unknown
kernel32.dll!00007ff986977614()	Unknown
ntdll.dll!00007ff9870626a1()	Unknown
@fujii
Copy link
Author

fujii commented Jan 23, 2023

I confirmed the following patch solves the crash bug.

diff --git a/lib/vtls/vtls.c b/lib/vtls/vtls.c
index dd0414ce7..889f6137d 100644
--- a/lib/vtls/vtls.c
+++ b/lib/vtls/vtls.c
@@ -315,11 +315,15 @@ static void cf_ctx_free(struct ssl_connect_data *ctx)
   }
 }
 
-static void cf_ctx_set_data(struct Curl_cfilter *cf,
+static struct Curl_easy *cf_ctx_set_data(struct Curl_cfilter *cf,
                             struct Curl_easy *data)
 {
-  if(cf->ctx)
+  struct Curl_easy *old_data = NULL;
+  if(cf->ctx) {
+    old_data = ((struct ssl_connect_data *)cf->ctx)->call_data;
     ((struct ssl_connect_data *)cf->ctx)->call_data = data;
+  }
+  return old_data;
 }
 
 static CURLcode ssl_connect(struct Curl_cfilter *cf, struct Curl_easy *data)
@@ -1786,9 +1790,9 @@ void *Curl_ssl_get_internals(struct Curl_easy *data, int sockindex,
     /* get first filter in chain, if any is present */
     cf = Curl_ssl_cf_get_ssl(data->conn->cfilter[sockindex]);
     if(cf) {
-      cf_ctx_set_data(cf, data);
+      struct Curl_easy *old_data = cf_ctx_set_data(cf, data);
       result = Curl_ssl->get_internals(cf->ctx, info);
-      cf_ctx_set_data(cf, NULL);
+      cf_ctx_set_data(cf, old_data);
     }
   }
   return result;

@fujii
Copy link
Author

fujii commented Jan 23, 2023

I was trying to create a simple program to reproduce the crash.
But, no luck so far. Maybe, there is another unknown condition to reproduce.

There is only one reproducible step I know at the moment.

  1. Download the prebuilt Windows WebKit binary (WebKit WinCairo port) and WebKitRequirements.
    https://trac.webkit.org/wiki/BuildingCairoOnWindows#DownloadbuildartifactsfromBuildbot
  2. set proxy env vars

set http_proxy=http://your-proxy:8080/
set https_proxy=%http_proxy%

  1. Start the browser
  2. It crashes.

@jay
Copy link
Member

jay commented Jan 23, 2023

@icing any idea what could be causing this?

@icing
Copy link
Contributor

icing commented Jan 23, 2023

@fujii thanks for all the details. Your patch will indeed solve the issue. Thinking about a better way to address this.

@jay we have, via the user callbacks, a recursive call into the filters and the current way of handling the call_data, e.g. the currently relevant easy handle, will clear it before the "outer" invocation is done.

@jay
Copy link
Member

jay commented Jan 23, 2023

Tricky. There's multi->in_callback if that helps.

edit: It's possible in_callback might not be set for all user callbacks, so maybe it won't help as much as I thought...

@icing
Copy link
Contributor

icing commented Jan 23, 2023

Indeed. And possibly, those could happen nested as well.

icing added a commit to icing/curl that referenced this issue Jan 23, 2023
- refs curl#10336 where the previous implementation cleared `data` so
  the outer invocation lost its data.
@icing
Copy link
Contributor

icing commented Jan 23, 2023

How about #10340 to address this?

jay pushed a commit that referenced this issue Jan 26, 2023
The previous implementation cleared `data` so the outer invocation lost
its data, which could lead to a crash.

Bug: #10336
Reported-by: Fujii Hironori

Closes #10340
@jay
Copy link
Member

jay commented Jan 26, 2023

@fujii A potential fix for this issue just landed in 9e93bd4. Please let us know if you can still reproduce with the fix. Thanks

@jay jay added the needs-info label Jan 26, 2023
@fujii
Copy link
Author

fujii commented Jan 26, 2023

I tested and confirmed it works fine. Thank you very much.

@fujii fujii closed this as completed Jan 26, 2023
@jay jay removed the needs-info label Jan 26, 2023
bch pushed a commit to bch/curl that referenced this issue Jul 19, 2023
The previous implementation cleared `data` so the outer invocation lost
its data, which could lead to a crash.

Bug: curl#10336
Reported-by: Fujii Hironori

Closes curl#10340
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants