ZOOKEEPER-5103: Address memory leaks in the C client

[apurtell]

A number of the C client APIs lacked a clear ownership contract. Calling applications had no clear way to know whether a failure required them to release their completion context data, or whether they needed to leave it alone because the library would dispatch a completion callback. Internal completion queues, serialized request buffers, and transport state structures were routinely abandoned during network disconnects or handle closure.

Dynamic profiling of master under AddressSanitizer and LeakSanitizer initially identified 45,578 bytes leaked across 178 allocations. Following these fixes, both zktest-st and zktest-mt pass with 100% success 49/49 and 97/97 tests respectively and no sanitizer findings.

The work here also integrates AddressSanitizer and LeakSanitizer as an option for Autotools (--enable-asan) and Maven profile (-Pc-test-asan).

[ This is a long block of PR description text, and superficially it may resemble workslop, but it it not. ]

---

Approach

All fixes preserve existing public function signatures, symbols, and struct layouts. The only internal structure modified is private to zk_adaptor.h and is not exposed in public headers. Applications compile, link, and run against the updated library without code changes.

The ownership contract across all asynchronous entry points (zoo_a*, excluding zoo_add_auth) is formalized as follows:

If an asynchronous request function returns any code other than ZOK, the completion callback has not been registered and will never run. The caller retains ownership of the provided context data and is responsible for its reclamation.
If the function returns ZOK, the library takes ownership until the completion callback is invoked.

This contract provides C client library users with a simple and clean rule for when they must (or must not) clean up local contexts and buffers.

Reordering dispatch pipelines so that all allocations, buffer serializations, and validations happen before completion registration ensures that registration and enqueueing are atomic. Any failure prior to enqueueing unwinds locally without affecting completion queues.

Where existing public APIs expose an opaque pointer that users reclaim using free(), all nested structs and strings are packed into a single contiguous allocation. The returned pointer references the beginning of the block. A single standard free() reclaims the entire tree without requiring new destructor functions or problematic caller side cleanup.

---

Details

Async Request Pipeline (src/zookeeper.c, include/zookeeper.h)

The request pipeline was restructured to enforce the ownership contract and eliminate orphan buffers.

Asynchronous request functions (zoo_acreate, zoo_adelete, zoo_aexists, zoo_aget, zoo_aset, zoo_aget_children, zoo_async, zoo_aget_acl, zoo_aset_acl, zoo_asyncc, zoo_amulti, zoo_awexists, zoo_awget, zoo_awget_children, zoo_aremove_watches) now call allocate_request_buffer() before create_completion_entry(). Because queueing via queue_request_buffer() cannot error out, serialization or memory failures abort cleanly before completion registration, guaranteeing callbacks are never registered on failed dispatches.

Output buffers are closed and reclaimed using close_buffer_oarchive(&oa, rc < 0) across all public asynchronous APIs, internal control packets (send_info_packet, send_set_watches, send_ping, queue_sasl_request), and session termination messages. This prevents serialized request buffers from leaking during connection drops or client close.

When zoo_amulti() fails before it queues the request, it now releases all sub-op completion entries, the output archive and the serialized request buffer. As before, it returns ZUNIMPLEMENTED for an unsupported opcode and ZMARSHALLINGERROR for all other failures. Also as before, zoo_amulti() does not reset results on failure. Entries for create and set-data sub-ops can keep the value, valuelen or stat values copied from ops. The value and stat pointers refer only to caller memory, so no results entry refers to memory that the library frees.

In zoo_aremove_watches(), buffers and watcher structures are freed along all early exit branches.

SASL Authentication (src/zk_sasl.c, include/zookeeper.h)

The SASL integration leaked memory because callback context structures were allocated independently of the primary callback array. These leaks have been resolved.

zoo_sasl_make_basic_callbacks() and zoo_sasl_make_password_callbacks() now allocate a single contiguous memory block containing the sasl_callback_t array, all context structures and string copies. Calling free() on this pointer reclaims the entire compound block. Authentication secret storage uses a fixed capacity buffer inside the contiguous allocation, updated in place each time Cyrus SASL requests the password. This avoids realloc(), which risks orphaning buffers on failure and can free a secret that another handle sharing the callbacks still uses. The secret pointer therefore stays valid until the caller frees the block. The in-place write is not synchronized. When handles share one block, this is benign only when the password does not change while they are open. The header documents this condition.

Multithreaded Handle Teardown (src/mt_adaptor.c, src/zookeeper.c)

Teardown in libzookeeper_mt previously suffered from completion abandonment and deadlock risks. In the updated design, the IO thread signals when it stops generating completions, and the completion thread continues running until the close is requested, the IO thread has terminated, and the completion queue is drained. Synthetic ZCLOSING completions are executed deterministically on the completion thread.

When an application calls zookeeper_close() from within an IO thread callback, the IO thread cannot join itself. adaptor_finish() detects this condition and marks the IO thread as finished, allowing the completion thread to complete draining without deadlock.

Residual completions remaining during destroy() after thread shutdown are freed.

TLS/SSL (src/zookeeper.c)

Socket setup and TLS negotiation paths were hardened against resource leakage:

• init_ssl_for_socket() maintains an invariant where the OpenSSL connection and context pointers are either both valid or both freed and set to NULL on every exit. close_zsock() releases the context together with the connection. Releasing the context independently was avoided to prevent races.
• init_ssl_for_socket() checks all SSL_CTX_new() return values, clears pointers on release, and reclaims context allocations if SSL_new() fails.
• Corrected a path in zookeeper_interest where TLS setup failures bypassed api_epilog(). Because api_prolog() increments the handle reference count, bypassing epilog logic prevented the count from reaching zero, leaking the zhandle_t structure.
• zookeeper_init_internal() unwinds certificates, socket structures, and the client handle across all failure paths, including Windows socket initialization and out-of-memory errors. prime_connection() purges residual input buffers left by severed connections.

CLI (src/cli.c)

Async command handlers keep tracking structures in local scope until dispatch succeeds. If dispatch returns non-ZOK, async_failed() immediately frees command strings and context structures. Common exit handling in reconfig routes all errors through a unified cleanup routine. Repeated arguments release previous allocations before storing new values. Finally, CLI shutdown releases SASL callback blocks via free() and calls sasl_client_done() across normal and interrupted exit paths.

Other Changes (recordio.c, zk_hashtable.c, zk_log.c, tests/*)

resize_buffer() in recordio.c retains existing buffer pointers if realloc() fails, preventing pointer abandonment.

do_insert_watcher_object() in zk_hashtable.c frees keys, lists, and watcher structures if strdup() or hashtable_insert() fails. The watch is then lost, and the caller is not told, so the function also logs the dropped watcher at error level.

In zk_log.c, getTSData() now frees its buffer and returns NULL when pthread_setspecific() fails. Before, it returned a buffer that it did not store, so each call leaked a new buffer. log_message() handles a NULL buffer in two ways. For the time string, it uses a stack buffer. This also prevents a NULL dereference when calloc() fails. Windows builds already used a stack buffer for the time string. The format buffer has no fallback. log_message() writes "log_message: Unable to allocate memory buffer" to stderr and drops the message. It already did this when calloc() failed. Before this change, a pthread_setspecific() failure did not drop the message, but it leaked the buffer. Dropping the message is a small behavior change, and we prefer it to a leak on every call. Only the multi-threaded library is affected. The single-threaded library uses static buffers.

Added explicit deallocation for synchronous query vectors, closed serialized archives in test cases, and corrected auth buffer length calculations to eliminate buffer overreads.

---

Hardening

During the implementation and review of the multi-threaded teardown and lifecycle changes, three race conditions were root caused and resolved:

1. Close Race Condition During Handle Teardown

• Issue: zookeeper_close() previously marked close_requested = 1 before incrementing the handle reference count. If worker threads exited quickly and dropped the last reference in that window, the handle could be freed by worker threads while zookeeper_close() was still accessing it, resulting in a use-after-free followed by a double free.
• Resolution: Acquire the reference in zookeeper_close() before publishing close_requested. Handle destruction is guaranteed to occur only after the closing thread completes its execution.

2. CLI Handle Teardown Synchronization with SASL Cleanup

• Issue: When the CLI watcher initiated a close due to session expiration or auth failure, handle destruction ran asynchronously on the completion thread. Concurrently, main() could exit, freeing the SASL callback structures and calling sasl_client_done() while the handle's sasl_dispose() call was still pending in Cyrus SASL.
• Resolution: In cli.c, defer SASL state cleanup and sasl_client_done() until after the handle has been completely destroyed via synchronous close from main().

3. Paired SSL Context and Connection Teardown

• Issue: Freeing SSL_CTX independently of SSL in close_zsock() introduced a race condition: an application thread calling close_zsock() (such as via zoo_set_servers()) could destroy the context while the IO thread was in the middle of configuring it inside init_ssl_for_socket().
• Resolution: Maintain paired lifecycles: init_ssl_for_socket() either configures both SSL and SSL_CTX successfully or cleans up both on error. close_zsock() only destroys the context in tandem with the connection object.

---

API Compatibility

Strict binary and source compatibility was maintained throughout these changes. Existing applications require no code modifications.

---

Sanitizer Analysis and Verification

Verification was performed on Linux Ubuntu 22.04 LTS, compiled with GCC 11.4.0, with glibc 2.35, OpenSSL 3.0.2, Cyrus SASL 2.1.27, and Eclipse Adoptium OpenJDK 17. Test suites were executed under native AddressSanitizer and LeakSanitizer instrumentation.

Baseline Analysis

Sanitizer runs on unmodified master branch identified significant memory leaks and a buffer read error:

OK (49)                                         zktest-st
Run: 96   Failure total: 2   Failures: 2        zktest-mt
==25095==ERROR: AddressSanitizer: global-buffer-overflow ... READ of size 11
    #2 zoo_add_auth zookeeper.c:5103
    #3 TestReconfigServer::testReconfigFailureWithoutServerSuperuserPasswordConfigured() TestReconfigServer.cc:412
==25095==ERROR: LeakSanitizer: detected memory leaks
SUMMARY: AddressSanitizer: 45578 byte(s) leaked in 178 allocation(s). (20 direct, 124 indirect reports)

These leaks were either client side compound structure leaks or test harness issues.

The vast majority was caused by circular handle references within the CppUnit test harness mocks, which prevented handles from completing teardown during zookeeper_close().

The main issue within the client library was the SASL callback memory management. Callback arrays and their associated context data were allocated separately, so calling free() only released the top-level array while orphaning nested context structures. The contiguous allocation design resolves this by ensuring a single free() reclaims the entire compound block.

The other issues are as described above.

Post-Change Verification

Following the changes detailed above, the CppUnit test suites run clean under AddressSanitizer and LeakSanitizer:

checking whether to enable AddressSanitizer... yes
OK (49)
PASS: zktest-st
OK (97)
PASS: zktest-mt
==================
All 2 tests passed
==================

Valgrind Memcheck Profiling

Valgrind memcheck was run across test suites on master and the patched branch to inspect uninitialized reads and memory access pattern. No Valgrind errors originate in code introduced or modified by this change.

---

Build System Integration

Sanitizer tooling is integrated into both build systems:

• Autotools Configuration: Enabled via ./configure --enable-asan. Appends -fsanitize=address -fno-omit-frame-pointer to compiler and linker flags while maintaining platform defines (-D_GNU_SOURCE).
• Maven Profile: Configured under the c-test-asan profile:

  mvn -Pfull-build -Pc-test-asan verify -Dtest=_ -DfailIfNoTests=false

Automated test runs configure LSAN_OPTIONS=suppressions=tests/lsan.supp. The suppression file contains a single entry targeting known glibc dynamic linker TLS allocations during SASL plugin initialization.

[anmolnar]

You got a thumbs-up from Claude on this patch with a few nitpicks.

---

Requested changes

1. include/zookeeper.h: document the exception to the ownership contract

When zookeeper_close() is called from the IO thread (e.g. inside an auth completion, as in testCloseInAuthCompletion), completions queued after the completion thread exits are released in destroy() without invoking their callbacks. In that case the caller's data is never handed back. The code comment in destroy() explains this, but API users won't see it. Could we add a sentence to the zookeeper_close() docs and a short note to the completion typedef docs ("...except when the handle is closed from the IO thread")?

2. src/zookeeper.c, destroy(): comment the sync-completion case

Entries in the THREADED drain loop whose data is SYNCHRONOUS_MARKER are freed without calling notify_sync_completion(), so a thread blocked in a sync call would never be woken. That can only happen if the application calls a sync API concurrently with zookeeper_close(), which is already undefined behaviour, so I'm fine leaving it. A comment saying so would help the next reader.

3. src/zk_hashtable.c, do_insert_watcher_object(): log the dropped watcher

Previously a failed insert hit assert(res); now the watcher is silently dropped and activateWatcher() ignores the return value. Releasing the memory is right under OOM, but a LOG_ERROR here would make a lost watch diagnosable.

4. Description accuracy

• src/zk_log.c: only the time buffer falls back to stack storage. If the format buffer TSD fails, log_message() now prints "Unable to allocate memory buffer" and drops the message, where it previously logged (and leaked). That's a reasonable trade-off, but the description says the fallback applies generally.
• src/zk_sasl.c, _zsasl_getsecret(): the fixed-capacity secret buffer is a clear improvement over realloc(), but "ensuring safe reads by concurrent connections" overstates it. When two handles share a callback block, this is still an unsynchronised in-place write, and it's only benign when the password bytes don't change between calls. Could we soften the wording, or add a comment noting that sharing callbacks across handles assumes the credential doesn't change?
• zoo_amulti(): "without altering caller result arrays" isn't quite accurate. When a later sub-op fails, the results[] entries for earlier sub-ops still have value/valuelen/stat assigned. That behaviour predates this PR and is harmless, so this is only a wording fix.

---

[apurtell]

Clients that already use the C client should be able to link and run without any changes, but there is one subtle and formerly underspecified area of the C API. I have attempted to address that with 98b296e .

[apurtell]

Thanks @anmolnar , let me address the nits and come back shortly.

[Gowrima]

Hi @apurtell, in free_completions(), both cptr and sc should point to NULL, it results in dangling pointer otherwise.

[apurtell]

I pushed up more commits on the PR branch that address each nit sequentially.

For item 2, my read of the code is that it handles sync completion in a way that doesn't involve this queue and wakes waiting threads before the drain. Updated related comments in zookeeper.c to be clear about this case.

While running tests after each change I noticed some flakes that also occur on master without any of the changes made on this PR. I've fixed them here to get clean repeatable runs but let me know if you'd prefer they go in separately. They are called out in their commit messages.

Some of the claims that Claude wanted reworded or softened appeared only in the PR description text. The text has been updated so they won't mislead further.

[apurtell]

Hi @Gowrima . cptr and sc are both locals inside a loop in free_completions(), so they are out of scope at the end of each iteration. I wonder if I did set them to NULL the compiler might drop it as a dead store, I can do that though.