Skip to content

sql: clear clnt->thd before handing the client back - fix for 'cldeadlock' test - #6173

Open
markhannum wants to merge 1 commit into
bloomberg:mainfrom
markhannum:fix-dangling-clnt-thd
Open

sql: clear clnt->thd before handing the client back - fix for 'cldeadlock' test#6173
markhannum wants to merge 1 commit into
bloomberg:mainfrom
markhannum:fix-dangling-clnt-thd

Conversation

@markhannum

Copy link
Copy Markdown
Contributor

clnt->thd points at the sqlthdstate the query ran under, which is alloca'd on the pool thread's stack. Nothing cleared it, so it outlives the frame.

cldeadlock reaches it by orphaning handles mid-statement. When the client vanishes send_columns() fails; for a non-select sqlinterfaces.c:4042 sets query_rc = CDB2ERR_IO_ERROR, and done_cb_evbuffer() returns on that value before setting clnt->done = 1. The connection is left with done == 0 and a stale clnt->thd, and the once-a-second watchdog scan dereferences it after the pool thread exits.

Adds clnt_detach_thd(), called before signal_clnt_as_done() in sqlengine_work_appsock() and sqlengine_work_lua_thread(). Skipped on nested replay, where the outer frame still owns thd. Not done on the RC_INTERNAL_RETRY path: enqueue_sql_query() has already cleared clnt->thd and the new owner may have freed clnt.

Fallout: reqlog_long_running_clnt() reads clnt->thd once rather than three times; recover_deadlock_flags() takes the sql_thread from TLS as recover_deadlock_flags_int() already does; osql_clean_sqlclntstate() preserves in_replay_nested across its bzero.

Fixes issue discovered by cldeadlock.

@roborivers roborivers left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

The first 10 failing tests are:
sc_resume_logicalsc_generated **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**

@markhannum
markhannum force-pushed the fix-dangling-clnt-thd branch from ab080e4 to abf4b7b Compare August 28, 2026 20:13

@roborivers roborivers left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

The first 10 failing tests are:
sc_truncate [db unavailable at finish]
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**

clnt->thd outlives the sqlthdstate it points at, which is alloca'd on the
pool thread's stack. Clear it before signal_clnt_as_done(). Hardening: the
common path is already covered by done_cb_evbuffer() marking clnt->done.

Signed-off-by: Mark Hannum <[email protected]>
@markhannum
markhannum force-pushed the fix-dangling-clnt-thd branch from abf4b7b to db0fc1e Compare August 29, 2026 15:30

@roborivers roborivers left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cbuild submission: Error ⚠.
Regression testing: Success ✓.

The first 10 failing tests are:
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants