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

Fix origin session cache double free #8330

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

duke8253
Copy link
Contributor

We started seeing some weird crashes after turning on origin session cache in production, and it seems like it was caused by SSL session being freed more than once. My guess is that since the current origin sessions stores the OpenSSL's session object directly, and let OpenSSL handle the freeing, when multiple threads uses the same session and terminates the session, the object gets freed while others are still using it. This commit reverts back to using d2i_SSL_SESSION and i2d_SSL_SESSION to convert sessions, since I don't like the idea of manually keeping track OpenSSL's internal ref counter. Also calls SSL_SESSION_free manually on the generated sessions as per OpenSSL's documentation.

SSL_SESSION_free() must only be called for SSL_SESSION objects, for which the reference count was explicitly incremented (e.g. by calling SSL_get1_session(), see SSL_get_session(3)) or when the SSL_SESSION object was generated outside a TLS handshake operation, e.g. by using d2i_SSL_SESSION(3). It must not be called on other SSL_SESSION objects, as this would cause incorrect reference counts and therefore program failures.

#0  0x0000000000000000 in ?? ()
#1  0x00007fe17e10f8e3 in ssl_security (s=s@entry=0x7fe16f162c00, op=op@entry=131077, bits=<optimized out>, nid=<optimized out>, 
    other=other@entry=0x7fe1740000fe) at ssl/ssl_cert.c:964
#2  0x00007fe17e1400d0 in tls_curve_allowed (s=s@entry=0x7fe16f162c00, curve=curve@entry=29, op=op@entry=131077) at ssl/t1_lib.c:263
#3  0x00007fe17e140230 in tls1_shared_group (nmatch=0, s=0x7fe16f162c00) at ssl/t1_lib.c:305
#4  tls1_shared_group (s=0x7fe16f162c00, nmatch=<optimized out>) at ssl/t1_lib.c:283
#5  0x00007fe17e10c147 in ssl3_ctrl (s=0x7fe16f162c00, cmd=<optimized out>, larg=0, parg=0x0) at ssl/s3_lib.c:3633
#6  0x0000000000795792 in SSLGetCurveNID (ssl=<optimized out>) at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLUtils.cc:2426
#7  0x0000000000599d65 in getSSLCurve (this=0x7fdf32c04f00)
    at /sd/workspace/src/git.vzbuilders.com/Edge/build/_build/build_release_posix-x86_64_gcc_8/trafficserver9.1/build/../../../../_scm/trafficserver9.1/iocore/net/P_SSLNetVConnection.h:300
#8  HttpSM::attach_client_session (this=0x7fdf32bb5490, client_vc=0x7fdf32a9bc40) at ../../../../../../_scm/trafficserver9.1/proxy/http/HttpSM.cc:564
#9  0x0000000000563e6b in Http1ClientSession::new_transaction (this=0x7fdf32a9b8e0)
    at ../../../../../../_scm/trafficserver9.1/proxy/http/Http1ClientSession.cc:469
#10 0x000000000075887d in ProxySession::do_api_callout (this=this@entry=0x7fdf32a9b8e0, id=id@entry=TS_HTTP_SSN_START_HOOK)
    at ../../../../../_scm/trafficserver9.1/proxy/ProxySession.cc:146
#11 0x0000000000566b71 in Http1ClientSession::new_connection(NetVConnection*, MIOBuffer*, IOBufferReader*) ()
    at ../../../../../../_scm/trafficserver9.1/proxy/http/Http1ClientSession.cc:207
#12 0x000000000055d869 in HttpSessionAccept::accept(NetVConnection*, MIOBuffer*, IOBufferReader*) ()
    at ../../../../../../_scm/trafficserver9.1/proxy/http/HttpSessionAccept.cc:59
#13 0x0000000000756528 in ProtocolProbeTrampoline::ioCompletionEvent (this=0x7fe172f71000, event=<optimized out>, edata=<optimized out>)
    at ../../../../../_scm/trafficserver9.1/proxy/ProtocolProbeSessionAccept.cc:151
#14 0x00000000007b567e in read_signal_and_update(int, UnixNetVConnection*) () at ../../../../../../_scm/trafficserver9.1/iocore/net/UnixNetVConnection.cc:83
#15 0x000000000078349e in SSLNetVConnection::net_read_io(NetHandler*, EThread*) ()
    at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLNetVConnection.cc:681
#16 0x00000000007a722e in NetHandler::process_ready_list (this=this@entry=0x7fe175ebf1b0) at ../../../../../../_scm/trafficserver9.1/iocore/net/UnixNet.cc:415
#17 0x00000000007a7590 in NetHandler::waitForActivity(long) () at ../../../../../../_scm/trafficserver9.1/iocore/net/UnixNet.cc:552
#18 0x00000000007f8ec3 in EThread::execute_regular (this=this@entry=0x7fe175ebaf00)
    at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/I_PriorityEventQueue.h:115
#19 0x00000000007f90f2 in execute (this=0x7fe175ebaf00) at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/UnixEThread.cc:364
#20 EThread::execute (this=0x7fe175ebaf00) at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/UnixEThread.cc:342
#21 0x00000000007f76da in spawn_thread_internal (a=0x7fe17b8b4340) at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/Thread.cc:92
#22 0x00007fe17d279ea5 in start_thread () from /lib64/libpthread.so.0
#23 0x00007fe17c57e9fd in clone () from /lib64/libc.so.6
#0  0x00007f4de2415e99 in get_error_values (inc=inc@entry=0, top=top@entry=0, file=file@entry=0x0, line=line@entry=0x0, data=data@entry=0x0, 
    flags=flags@entry=0x0) at crypto/err/err.c:529
#1  0x00007f4de2416233 in ERR_peek_error () at crypto/err/err.c:473
#2  0x00007f4de27e9bbe in SSL_get_error (s=s@entry=0x7f4dd0da8c00, i=i@entry=-1) at ssl/ssl_lib.c:3571
#3  0x000000000079a943 in SSLAccept(ssl_st*) () at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLUtils.cc:1925
#4  0x00000000007805a6 in SSLNetVConnection::sslServerHandShakeEvent(int&) () at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLNetVConnection.cc:1253
#5  0x0000000000783ea5 in SSLNetVConnection::sslStartHandShake(int, int&) () at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLNetVConnection.cc:1060
#6  0x0000000000782d42 in SSLNetVConnection::net_read_io(NetHandler*, EThread*) ()
    at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLNetVConnection.cc:569
#7  0x00000000007a722e in NetHandler::process_ready_list (this=this@entry=0x7f4dda082070) at ../../../../../../_scm/trafficserver9.1/iocore/net/UnixNet.cc:415
#8  0x00000000007a7590 in NetHandler::waitForActivity(long) () at ../../../../../../_scm/trafficserver9.1/iocore/net/UnixNet.cc:552
#9  0x00000000007f8ec3 in EThread::execute_regular (this=this@entry=0x7f4dda07ddc0)
    at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/I_PriorityEventQueue.h:115
#10 0x00000000007f90f2 in execute (this=0x7f4dda07ddc0) at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/UnixEThread.cc:364
#11 EThread::execute (this=0x7f4dda07ddc0) at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/UnixEThread.cc:342
#12 0x00000000007f76da in spawn_thread_internal (a=0x7f4ddfeb4540) at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/Thread.cc:92
#13 0x00007f4de194aea5 in start_thread () from /lib64/libpthread.so.0
#14 0x00007f4de0c4f9fd in clone () from /lib64/libc.so.6
#0  0x00007f7f90233e75 in CRYPTO_UP_REF (lock=<error reading variable: Cannot access memory at address 0x14f>, ret=<synthetic pointer>, val=0xbf)
    at include/internal/refcount.h:34
#1  X509_up_ref (x=0xffffffffffffffff) at crypto/x509/x509_set.c:103
#2  0x00007f7f9057498e in ssl_session_dup (src=src@entry=0x7f7f8150d300, ticket=ticket@entry=0) at ssl/ssl_sess.c:149
#3  0x00007f7f905862c4 in tls_process_new_session_ticket (s=0x7f7f81551800, pkt=0x7f7f85b00d30) at ssl/statem/statem_clnt.c:2619
#4  0x00007f7f90587e55 in ossl_statem_client_process_message (s=0x7f7f81551800, pkt=<optimized out>) at ssl/statem/statem_clnt.c:1060
#5  0x00007f7f90581a23 in read_state_machine (s=0x7f7f81551800) at ssl/statem/statem.c:636
#6  state_machine (s=0x7f7f81551800, server=0) at ssl/statem/statem.c:434
#7  0x00007f7f9055aa19 in ssl3_read_bytes (s=0x7f7f81551800, type=23, recvd_type=0x0, buf=0x7f7d40f22000 <Address 0x7f7d40f22000 out of bounds>, len=8192, 
    peek=0, readbytes=0x7f7f85b00ef8) at ssl/record/rec_layer_s3.c:1658
#8  0x00007f7f905613e5 in ssl3_read_internal (s=0x7f7f81551800, buf=0x7f7d40f22000, len=8192, peek=0, readbytes=0x7f7f85b00ef8) at ssl/s3_lib.c:4464
#9  0x00007f7f9056bcfa in ssl_read_internal (readbytes=0x7f7f85b00ef8, num=8192, buf=0x7f7d40f22000, s=0x7f7f81551800) at ssl/ssl_lib.c:1769
#10 ssl_read_internal (s=0x7f7f81551800, buf=0x7f7d40f22000, num=8192, readbytes=0x7f7f85b00ef8) at ssl/ssl_lib.c:1732
#11 0x00007f7f9056bde5 in SSL_read (s=s@entry=0x7f7f81551800, buf=buf@entry=0x7f7d40f22000, num=num@entry=8192) at ssl/ssl_lib.c:1783
#12 0x0000000000799d6b in SSLReadBuffer(ssl_st*, void*, long, long&) () at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLUtils.cc:1844
#13 0x000000000077d5dd in ssl_read_from_net(SSLNetVConnection*, EThread*, long&) ()
    at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLNetVConnection.cc:281
#14 0x0000000000782ea3 in SSLNetVConnection::net_read_io(NetHandler*, EThread*) ()
    at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLNetVConnection.cc:671
#15 0x00000000007a722e in NetHandler::process_ready_list (this=this@entry=0x7f7f88400230) at ../../../../../../_scm/trafficserver9.1/iocore/net/UnixNet.cc:415
#16 0x00000000007a7590 in NetHandler::waitForActivity(long) () at ../../../../../../_scm/trafficserver9.1/iocore/net/UnixNet.cc:552
#17 0x00000000007f8ec3 in EThread::execute_regular (this=this@entry=0x7f7f883fbf80)
    at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/I_PriorityEventQueue.h:115
#18 0x00000000007f90f2 in execute (this=0x7f7f883fbf80) at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/UnixEThread.cc:364
#19 EThread::execute (this=0x7f7f883fbf80) at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/UnixEThread.cc:342
#20 0x00000000007f76da in spawn_thread_internal (a=0x7f7f8dcb4380) at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/Thread.cc:92
#21 0x00007f7f8f6cfea5 in start_thread () from /lib64/libpthread.so.0
#22 0x00007f7f8e9d49fd in clone () from /lib64/libc.so.6
#0  OPENSSL_sk_free (st=0x1) at crypto/stack/stack.c:376
#1  OPENSSL_sk_free (st=0x1) at crypto/stack/stack.c:372
#2  0x00007f99b22204f4 in sk_void_free (sk=<optimized out>) at include/openssl/crypto.h:89
#3  CRYPTO_free_ex_data (class_index=class_index@entry=2, obj=obj@entry=0x7f99a3b0ea00, ad=ad@entry=0x7f99a3b0ec00) at crypto/ex_data.c:361
#4  0x00007f99b25dc738 in SSL_SESSION_free (ss=0x7f99a3b0ea00) at ssl/ssl_sess.c:759
#5  SSL_SESSION_free (ss=0x7f99a3b0ea00) at ssl/ssl_sess.c:747
#6  0x00000000007915aa in ~SSLOriginSession (this=0x7f99aa11d020, __in_chrg=<optimized out>)
    at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLSessionCache.h:198
#7  SSLOriginSessionCache::insert_session(std::string const&, ssl_session_st*, ssl_st*) ()
    at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLSessionCache.cc:338
#8  0x0000000000772dcf in ssl_new_session_callback(ssl_st*, ssl_session_st*) () at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLClientUtils.cc:166
#9  0x00007f99b25d5dda in ssl_update_cache (s=s@entry=0x7f99a8545000, mode=mode@entry=1) at ssl/ssl_lib.c:3507
#10 0x00007f99b25ee692 in tls_process_new_session_ticket (s=0x7f99a8545000, pkt=<optimized out>) at ssl/statem/statem_clnt.c:2742
#11 0x00007f99b25efe55 in ossl_statem_client_process_message (s=0x7f99a8545000, pkt=<optimized out>) at ssl/statem/statem_clnt.c:1060
#12 0x00007f99b25e9a23 in read_state_machine (s=0x7f99a8545000) at ssl/statem/statem.c:636
#13 state_machine (s=0x7f99a8545000, server=0) at ssl/statem/statem.c:434
#14 0x00007f99b25c2a19 in ssl3_read_bytes (s=0x7f99a8545000, type=23, recvd_type=0x0, buf=0x7f9764704000 <Address 0x7f9764704000 out of bounds>, len=8192, 
    peek=0, readbytes=0x7f99ab279ef8) at ssl/record/rec_layer_s3.c:1658
#15 0x00007f99b25c93e5 in ssl3_read_internal (s=0x7f99a8545000, buf=0x7f9764704000, len=8192, peek=0, readbytes=0x7f99ab279ef8) at ssl/s3_lib.c:4464
#16 0x00007f99b25d3cfa in ssl_read_internal (readbytes=0x7f99ab279ef8, num=8192, buf=0x7f9764704000, s=0x7f99a8545000) at ssl/ssl_lib.c:1769
#17 ssl_read_internal (s=0x7f99a8545000, buf=0x7f9764704000, num=8192, readbytes=0x7f99ab279ef8) at ssl/ssl_lib.c:1732
#18 0x00007f99b25d3de5 in SSL_read (s=s@entry=0x7f99a8545000, buf=buf@entry=0x7f9764704000, num=num@entry=8192) at ssl/ssl_lib.c:1783
#19 0x0000000000799d6b in SSLReadBuffer(ssl_st*, void*, long, long&) () at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLUtils.cc:1844
#20 0x000000000077d5dd in ssl_read_from_net(SSLNetVConnection*, EThread*, long&) ()
    at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLNetVConnection.cc:281
#21 0x0000000000782ea3 in SSLNetVConnection::net_read_io(NetHandler*, EThread*) ()
    at ../../../../../../_scm/trafficserver9.1/iocore/net/SSLNetVConnection.cc:671
#22 0x00000000007a722e in NetHandler::process_ready_list (this=this@entry=0x7f99ad3ff9f0) at ../../../../../../_scm/trafficserver9.1/iocore/net/UnixNet.cc:415
#23 0x00000000007a7590 in NetHandler::waitForActivity(long) () at ../../../../../../_scm/trafficserver9.1/iocore/net/UnixNet.cc:552
#24 0x00000000007f8ec3 in EThread::execute_regular (this=this@entry=0x7f99ad3fb740)
    at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/I_PriorityEventQueue.h:115
#25 0x00000000007f90f2 in execute (this=0x7f99ad3fb740) at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/UnixEThread.cc:364
#26 EThread::execute (this=0x7f99ad3fb740) at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/UnixEThread.cc:342
#27 0x00000000007f76da in spawn_thread_internal (a=0x7f99afc4e2c0) at ../../../../../../_scm/trafficserver9.1/iocore/eventsystem/Thread.cc:92
#28 0x00007f99b1737ea5 in start_thread () from /lib64/libpthread.so.0
#29 0x00007f99b0a3c9fd in clone () from /lib64/libc.so.6

@duke8253 duke8253 added this to the 10.0.0 milestone Sep 16, 2021
@duke8253 duke8253 self-assigned this Sep 16, 2021
@duke8253 duke8253 force-pushed the master-orig_sess_fix branch 3 times, most recently from 341fcee to c5ce17b Compare September 16, 2021 20:39
@duke8253
Copy link
Contributor Author

[approve ci autest]

@duke8253
Copy link
Contributor Author

[approve ci clang-analyzer]

Copy link
Contributor

@masaori335 masaori335 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good 👍 Commented some nitpicks.

remove and free sessions that are timed out
free sessions that failed to be set, and free sessions when done
Copy link
Contributor

@masaori335 masaori335 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable.

@duke8253 duke8253 merged commit f4274a8 into apache:master Sep 28, 2021
@masaori335
Copy link
Contributor

@duke8253 do we need to backport this to the 9.2.0 branch?

@masaori335 masaori335 added this to In progress in 9.2.x Branch and Release via automation Jan 24, 2022
@zwoop zwoop moved this from In progress to Done in 9.2.x Branch and Release Jan 25, 2022
zwoop pushed a commit that referenced this pull request Jan 25, 2022
remove and free sessions that are timed out
free sessions that failed to be set, and free sessions when done

(cherry picked from commit f4274a8)
@zwoop
Copy link
Contributor

zwoop commented Jan 25, 2022

Cherry-picked to v9.2.x

@zwoop zwoop removed this from Done in 9.2.x Branch and Release Jan 25, 2022
@zwoop zwoop modified the milestones: 10.0.0, 9.2.0 Jan 25, 2022
moonchen pushed a commit to moonchen/trafficserver that referenced this pull request Mar 7, 2022
* asf/master: (47 commits)
  Doc: cleanup build errors. (apache#8377)
  Doc: Add proxy.config.cacvhe.mutex_retry_delay (apache#8376)
  Adds support for TCP_NOTSENT_LOWAT sockopt (apache#8354)
  Added support for promoting internal (plugin-initiated) requests. (apache#8363)
  Removed references to the throttle option from the slice plugin. (apache#8373)
  Pre-warming TLS Tunnel (apache#7661)
  Traffic Dump: update json-schema for new tuple requirements (apache#8370)
  TSSslSecretSet: Update SSL_CTX TLS Secrets (apache#8368)
  Added support for verifying cacheability before attempting to force an object into cache (apache#8364)
  [doc] Add a note for TSLifecycleHookAdd. Warn users that a contp could eventually be executed in a ET_NET when it was originally scheduled in the ET_TASK. (apache#8344)
  check size of session, and free sessions the ATS way (apache#8330)
  Locking around SSLSecret::secret_map access (apache#8358)
  Stabilize regex_revalidate Au test. (apache#559) (apache#8360)
  change MemArena::make test to remove memory leak (apache#8352)
  free sessions when timeout (apache#8356)
  test_MMH: fix memory leak in unit test (apache#8357)
  crash fix (apache#8268)
  remove unused RecConfigFileEntry struct (apache#8353)
  Rename outbound_conntrack to global_outbound_conntrack to reduce confusion. (apache#8343)
  remove unused RecConfigFileEntry from RecConfigParse (apache#8348)
  ...
moonchen pushed a commit to moonchen/trafficserver that referenced this pull request Mar 17, 2022
* asf/9.2.x:
  Updated ChangeLog
  Add SSLSessionDup for older OpenSSL and BoringSSL (apache#8578)
  use shared pointer to help with high memory utilization (apache#8498)
  Commenting TSHttpTxnCacheLookupStatusGet need_to_revalidate (apache#8621)
  check size of session, and free sessions the ATS way (apache#8330)
  free sessions when timeout (apache#8356)
  Fix 32bit build failure on Odroid Xu-4 (apache#8626)
  TSHttpTxnCacheLookupStatusGet: call need_to_revalidate (apache#8617)
  SNIConfig (tunnel_route): Change the way we extract matched subgroups from the server name. (apache#8589)
  fix for collapsed forwarding ink_abort for CacheHitFresh fail (apache#8613)
  Do not turn off cache for internal requests (apache#8266)
  Rate Limit Plugin: Re-enable VConnection when SNI is empty (apache#8625)
  Removes hard dependency on having perl installed (apache#8611)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants