perf: optimize SCRAM-SHA-256 auth with cached EVP_MD and verifier cache#5689
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a fast SCRAM-SHA-256 HMAC/PBKDF2 implementation and opaque incremental HMAC API, integrates them into SCRAM flows with a per-thread plaintext-password verifier cache and runtime invalidation, adds a TAP test to validate cache invalidation, and introduces a PostgreSQL connect benchmark tool. ChangesSCRAM: fast HMAC/PBKDF2, verifier cache, and integration
Tools / Benchmark
Sequence DiagramsequenceDiagram
participant Client
participant SCRAM_Module
participant Verifier_Cache
participant Crypto_Layer
Client->>SCRAM_Module: build_server_first_message(password)
SCRAM_Module->>Verifier_Cache: lookup(password)
alt cache hit
Verifier_Cache-->>SCRAM_Module: cached verifier
SCRAM_Module->>SCRAM_Module: parse verifier -> ScramState
else cache miss
Verifier_Cache-->>SCRAM_Module: miss
SCRAM_Module->>Crypto_Layer: scram_fast_SaltedPassword(password, salt, iter)
Crypto_Layer->>Crypto_Layer: iterative HMAC rounds -> salted_password
Crypto_Layer-->>SCRAM_Module: salted_password
SCRAM_Module->>Crypto_Layer: scram_fast_ClientKey(salted_password)
Crypto_Layer-->>SCRAM_Module: client_key
SCRAM_Module->>Crypto_Layer: scram_fast_ServerKey(salted_password)
Crypto_Layer-->>SCRAM_Module: server_key
SCRAM_Module->>Verifier_Cache: store(verifier for password)
SCRAM_Module->>SCRAM_Module: parse generated verifier -> ScramState
end
SCRAM_Module-->>Client: server-first-message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an optimized SCRAM-SHA-256 implementation designed to reduce overhead in OpenSSL 3.x by caching EVP_MD objects. It also implements a thread-local cache for SCRAM verifiers to avoid redundant PBKDF2 calculations when using plaintext passwords. Review feedback identifies a potential memory leak in the thread-local cache because allocated strings are not freed upon thread termination, and suggests implementing an eviction policy for the fixed-size cache to handle more than 64 unique users per thread.
| static __thread struct scram_verifier_cache_entry scram_verifier_cache[SCRAM_VERIFIER_CACHE_SLOTS]; | ||
| static __thread int scram_verifier_cache_count = 0; |
There was a problem hiding this comment.
The scram_verifier_cache uses strdup to allocate memory for password and verifier strings. Since this is a __thread variable, the allocated memory will be leaked when a thread terminates, as there is no mechanism provided to free these strings. In a long-running process like ProxySQL, this can lead to memory accumulation if threads are recycled or frequently created and destroyed.
| if (scram_verifier_cache_count >= SCRAM_VERIFIER_CACHE_SLOTS) | ||
| return false; |
There was a problem hiding this comment.
The SCRAM verifier cache has a fixed size of 64 slots and lacks an eviction policy (e.g., LRU). Once the cache is full, no new entries can be added, and stale entries (e.g., from users whose passwords have changed) will continue to occupy slots indefinitely. This may lead to sub-optimal performance for threads handling a large number of unique users. Consider implementing a simple eviction strategy.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deps/libscram/src/scram.c (1)
51-84: 💤 Low valueThread-local cache lacks cleanup on thread exit — bounded memory leak.
The cache uses
strdup()to copy passwords and verifiers but provides no destructor to free entries when threads exit. With ProxySQL's fixed thread pool this is bounded (64 entries × ~200 bytes × N threads), but entries for changed passwords also persist indefinitely.Consider documenting this trade-off, or adding a
pthread_key_create()destructor for cleanup if thread churn becomes a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deps/libscram/src/scram.c` around lines 51 - 84, The thread-local cache (scram_verifier_cache, scram_verifier_cache_count) stores strdup'ed strings in scram_cache_store but never frees them on thread exit; add a proper cleanup path by registering a pthread key destructor (via pthread_key_create + pthread_once for one-time init) that runs on thread teardown to iterate scram_verifier_cache entries, free each password and verifier, and reset scram_verifier_cache_count to 0; ensure the destructor is associated with the thread-local storage so existing code using scram_cache_lookup and scram_cache_store needs no other changes, or alternatively provide a public scram_cache_cleanup() function and call it from the destructor/when threads are joined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@deps/libscram/src/scram.c`:
- Around line 51-84: The thread-local cache (scram_verifier_cache,
scram_verifier_cache_count) stores strdup'ed strings in scram_cache_store but
never frees them on thread exit; add a proper cleanup path by registering a
pthread key destructor (via pthread_key_create + pthread_once for one-time init)
that runs on thread teardown to iterate scram_verifier_cache entries, free each
password and verifier, and reset scram_verifier_cache_count to 0; ensure the
destructor is associated with the thread-local storage so existing code using
scram_cache_lookup and scram_cache_store needs no other changes, or
alternatively provide a public scram_cache_cleanup() function and call it from
the destructor/when threads are joined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0067b1c7-e810-4922-bfbe-0dd10e5308de
📒 Files selected for processing (3)
deps/libscram/include/scram_hmac_cached.hdeps/libscram/src/scram.cdeps/libscram/src/scram_hmac_cached.c
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_,PgSQL_,ProxySQL_)
Member variables should use snake_case naming
Constants and macros should use UPPER_SNAKE_CASE naming
C++17 is required; use conditional compilation via#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31, etc. for feature-gated code
Use jemalloc for memory allocation
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
deps/libscram/include/scram_hmac_cached.h
🪛 Clang (14.0.6)
deps/libscram/src/scram_hmac_cached.c
[error] 1-1: 'scram_hmac_cached.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (14)
deps/libscram/src/scram.c (8)
86-118: LGTM!The
build_scram_verifierfunction has proper memory management with correct cleanup on all error paths.
656-729: LGTM!The migration to
scram_fast_*API is correct. The HMAC context functions handle NULL internally, so the deferred NULL check pattern is safe here.
731-772: LGTM!Correct migration to
scram_fast_*API with proper error handling.
989-1005: LGTM!Correct usage of the new key derivation functions with preserved error handling.
1112-1134: LGTM!The cache integration is well-structured with graceful degradation when the cache is full. Memory management for the temporary verifier string is correct.
1182-1226: LGTM!
compute_server_signaturecorrectly migrated to the new API.
1277-1324: LGTM!
verify_client_proofcorrectly migrated with proper cleanup paths.
1374-1378: LGTM!
scram_verify_plain_passwordcorrectly migrated to usescram_fast_SaltedPasswordandscram_fast_ServerKey.deps/libscram/include/scram_hmac_cached.h (1)
1-37: LGTM!Well-designed header with proper include guards, clear documentation, and a minimal API surface. The function signatures correctly use portable types (
uint8_t,size_t).deps/libscram/src/scram_hmac_cached.c (5)
17-36: LGTM!The EVP_MD caching approach is sound. Using
pthread_onceensures thread-safe one-time initialization. IfEVP_MD_fetchfails, returning NULL is correctly propagated to callers which handle it appropriately.
44-132: LGTM!Well-designed HMAC context API with consistent error handling. All functions are defensive against NULL inputs and properly propagate errors.
136-188: LGTM!PBKDF2 implementation is correct per RFC 5802. The algorithm properly computes
U1 = HMAC(password, salt || INT(1))and subsequentUi = HMAC(password, U(i-1))with XOR accumulation.
190-261: LGTM!Hash and key derivation functions correctly implement the RFC 5802 SCRAM primitives with proper error handling.
1-1: [Rewritten review comment]
[Classification tag]
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deps/libscram/src/scram.c`:
- Around line 682-683: The call to scram_fast_hmac_create() must be NULL-checked
before any subsequent HMAC calls; update the sites in scram.c (including the one
at the shown diff and the similar sites around the 725–750 region) to verify the
returned scram_fast_hmac_ctx* is non-NULL immediately after
scram_fast_hmac_create(), and if NULL avoid calling scram_fast_hmac_init() or
scram_fast_hmac_error() — instead propagate an error/cleanup (return an error
code or goto the existing cleanup path) and log the failure using available
logging routines so the OOM/OpenSSL setup failure is handled safely.
- Around line 58-66: The cache currently stores recoverable plaintext passwords
in struct scram_verifier_cache_entry (password field) via strdup(password),
increasing secret retention; change the cache to key entries by a
non-recoverable fingerprint (e.g., HMAC/SHA256 of the password plus salt)
instead of storing the plaintext, stop calling strdup(password) into
scram_verifier_cache[].password, and ensure any existing plaintext or verifier
buffers are securely scrubbed (overwrite with zeros) before free; update logic
that uses scram_verifier_cache_count, scram_verifier_cache,
scram_cache_generation and scram_my_generation to index by the fingerprint key
and to invalidate/scrub entries on eviction or generation bump.
In `@tools/bench_connect.c`:
- Around line 37-39: The code performs multiple heap allocations (e.g.,
arg->connect_us, arg->close_us, arg->total_us and other mallocs at later sites)
without checking for NULL; update each allocation site to check the return
value, handle OOM by freeing any earlier successful allocations and returning an
error (or exiting cleanly) instead of dereferencing NULL, and ensure any caller
(e.g., the function that allocates these arrays in bench_connect.c) observes the
failure so resources are not used; reference the specific allocation targets
arg->connect_us, arg->close_us, arg->total_us and the other malloc results noted
in the review and add proper NULL checks and cleanup paths.
- Around line 171-176: The min/p50 for close_us are being read from the unsorted
close_us array (uses close_us[0] and close_us[n/2]) which yields incorrect
stats; follow the pattern used for connect_us/total_us by allocating a new array
(e.g., double *sorted_close), memcpy close_us into it, qsort(sorted_close, n,
sizeof(double), cmp_double), then use sorted_close[0] and sorted_close[n/2]
where the code currently reads close_us[0]/close_us[n/2]; remember to free
sorted_close when done and keep naming consistent with sorted_conn/sorted_total.
- Around line 98-115: Validate and clamp all numeric CLI inputs: ensure
iterations is between 1 and MAX_ITERS, threads >= 1, warmup >= 0 (and warmup <=
iterations if intended), and port is in a valid range; reject or error on
invalid values (e.g., "--threads 0" or negative numbers). Before computing any
product like total_ops = threads * iterations (used later to derive n and that
can overflow), perform the multiplication in an unsigned wider type (e.g.,
uint64_t/size_t) and explicitly check for overflow or that total_ops > 0; ensure
any derived n is > 0 before doing divisions to avoid divide-by-zero. Return a
non-zero error when validation fails and add clear stderr messages referencing
the offending flag (iterations, threads, warmup, port) so downstream code (uses
of n and indexing into per-thread arrays) never sees invalid or overflowing
sizes.
- Around line 136-139: Check and handle return values from pthread_create and
pthread_join around the loop that spawns thread_worker using tids, args and
threads: capture each pthread_create return value, and on non-zero log an error
(including errno via strerror if desired), clean up any allocated resources and
exit or return a failure code instead of proceeding; similarly capture each
pthread_join return value and handle join failures (log and abort/cleanup) to
avoid inconsistent benchmark state. Ensure the error handling occurs in the same
scope where tids and args are used so threads are not assumed to have started or
completed when they have not.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a190bae4-03a3-41a7-872f-5d9784ddde97
📒 Files selected for processing (8)
deps/libscram/include/scram_hmac_cached.hdeps/libscram/src/scram.clib/ProxySQL_Admin.cpptest/tap/groups/groups.jsontest/tap/tests/pgsql-scram_cache_invalidation-t.cpptools/.gitignoretools/Makefiletools/bench_connect.c
✅ Files skipped from review due to trivial changes (2)
- tools/.gitignore
- test/tap/groups/groups.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu24,-tap-genai-gcov)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: run / trigger
- GitHub Check: claude-review
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{cpp,h,hpp}: Class names should use PascalCase with protocol prefixes (MySQL_,PgSQL_,ProxySQL_)
Member variables should use snake_case naming
Constants and macros should use UPPER_SNAKE_CASE naming
C++17 is required; use conditional compilation via#ifdef PROXYSQLGENAI,#ifdef PROXYSQL31, etc. for feature-gated code
Use jemalloc for memory allocation
Use pthread mutexes for synchronization andstd::atomic<>for counters
Files:
test/tap/tests/pgsql-scram_cache_invalidation-t.cpplib/ProxySQL_Admin.cppdeps/libscram/include/scram_hmac_cached.h
test/tap/tests/*-t.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should follow naming pattern
test_*.cppor*-t.cppintest/tap/tests/
Files:
test/tap/tests/pgsql-scram_cache_invalidation-t.cpp
lib/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
One class per file typically in
lib/directory
Files:
lib/ProxySQL_Admin.cpp
🧠 Learnings (4)
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/pgsql-scram_cache_invalidation-t.cpp
📚 Learning: 2026-04-17T16:31:58.196Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to **/*.{cpp,h,hpp} : C++17 is required; use conditional compilation via `#ifdef PROXYSQLGENAI`, `#ifdef PROXYSQL31`, etc. for feature-gated code
Applied to files:
lib/ProxySQL_Admin.cpp
📚 Learning: 2026-04-01T21:27:03.216Z
Learnt from: wazir-ahmed
Repo: sysown/proxysql PR: 5557
File: test/tap/tests/unit/gtid_set_unit-t.cpp:14-17
Timestamp: 2026-04-01T21:27:03.216Z
Learning: In ProxySQL's unit test directory (test/tap/tests/unit/), test_globals.h and test_init.h are only required for tests that depend on the ProxySQL runtime globals/initialization (i.e., tests that exercise components linked against libproxysql.a). Pure data-structure or utility tests (e.g., ezoption_parser_unit-t.cpp, gtid_set_unit-t.cpp, gtid_trxid_interval_unit-t.cpp) only need tap.h and the relevant project header — omitting test_globals.h and test_init.h is correct and intentional in these cases.
Applied to files:
lib/ProxySQL_Admin.cpp
📚 Learning: 2026-04-17T16:31:58.196Z
Learnt from: CR
Repo: sysown/proxysql PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T16:31:58.196Z
Learning: Applies to test/tap/tests/unit/**/*.cpp : Unit tests must use `test_globals.h` and `test_init.h` and link against `libproxysql.a` via the custom test harness
Applied to files:
lib/ProxySQL_Admin.cpp
🪛 checkmake (0.3.2)
tools/Makefile
[warning] 25-25: Required target "all" is missing from the Makefile.
(minphony)
[warning] 25-25: Required target "test" is missing from the Makefile.
(minphony)
🪛 Clang (14.0.6)
test/tap/tests/pgsql-scram_cache_invalidation-t.cpp
[error] 19-19: 'string' file not found
(clang-diagnostic-error)
tools/bench_connect.c
[error] 1-1: 'stdio.h' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.20.0)
tools/bench_connect.c
[warning] 20-20: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 132-132: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 133-133: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 134-134: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 135-135: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 139-139: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 152-152: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 153-153: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 154-154: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 155-155: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 156-156: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 157-157: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 158-158: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 168-168: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 173-173: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 173-173: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 174-174: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 174-174: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 175-175: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 176-176: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 180-180: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 181-181: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 182-182: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 200-200: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 204-204: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 206-206: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 207-207: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 209-209: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 213-213: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 224-224: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 228-228: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 230-230: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 230-230: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 232-232: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 236-236: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[error] 153-153: If memory allocation fails
(nullPointerArithmeticOutOfMemory)
[error] 154-154: If memory allocation fails
(nullPointerArithmeticOutOfMemory)
[error] 155-155: If memory allocation fails
(nullPointerArithmeticOutOfMemory)
🔇 Additional comments (3)
tools/Makefile (1)
21-23: Looks good for benchmark target integration.Line 21 and Line 26 cleanly add build/cleanup coverage for
bench_connect, which matches the PR objective.Also applies to: 26-26
lib/ProxySQL_Admin.cpp (2)
41-41: Good dependency wiring for SCRAM cache invalidation.Including
scram_hmac_cached.hhere is the right integration point for the new cache-invalidation hook.
6141-6143: Nice cache-coherency fix in the PgSQL users refresh path.Hooking
scram_cache_invalidate()into__refresh_pgsql_usersensures SCRAM verifier caches are refreshed when credentials are reloaded.
| struct scram_verifier_cache_entry { | ||
| char *password; /* plaintext password (key) */ | ||
| char *verifier; /* SCRAM-SHA-256$... verifier (value) */ | ||
| }; | ||
|
|
||
| static uint64_t scram_cache_generation = 0; | ||
| static __thread uint64_t scram_my_generation = 0; | ||
| static __thread struct scram_verifier_cache_entry scram_verifier_cache[SCRAM_VERIFIER_CACHE_SLOTS]; | ||
| static __thread int scram_verifier_cache_count = 0; |
There was a problem hiding this comment.
Avoid caching recoverable plaintext passwords in TLS state.
strdup(password) makes each worker thread keep its own heap copy of user passwords until invalidation or thread teardown. That is a significant secret-retention increase for a perf-only optimization. Prefer keying the cache with a non-recoverable fingerprint and scrub both cached secrets before freeing them.
Also applies to: 68-75, 96-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deps/libscram/src/scram.c` around lines 58 - 66, The cache currently stores
recoverable plaintext passwords in struct scram_verifier_cache_entry (password
field) via strdup(password), increasing secret retention; change the cache to
key entries by a non-recoverable fingerprint (e.g., HMAC/SHA256 of the password
plus salt) instead of storing the plaintext, stop calling strdup(password) into
scram_verifier_cache[].password, and ensure any existing plaintext or verifier
buffers are securely scrubbed (overwrite with zeros) before free; update logic
that uses scram_verifier_cache_count, scram_verifier_cache,
scram_cache_generation and scram_my_generation to index by the fingerprint key
and to invalidate/scrub entries on eviction or generation bump.
| scram_fast_hmac_ctx* ctx = scram_fast_hmac_create(); | ||
|
|
There was a problem hiding this comment.
Guard scram_fast_hmac_create() before the first HMAC operation.
deps/libscram/src/scram_hmac_cached.c returns NULL when the context allocation/OpenSSL setup fails. This path currently falls through to scram_fast_hmac_init() and then formats scram_fast_hmac_error(ctx) with a maybe-null context, which makes the OOM/failure path brittle. The same guard should be added to the other new HMAC call sites in this file as well.
Suggested fix
const char* errstr = NULL;
scram_fast_hmac_ctx* ctx = scram_fast_hmac_create();
+ if (!ctx) {
+ snprintf(errorBuffer, MAX_ERROR_LENGTH, "could not allocate HMAC context");
+ goto failed;
+ }Also applies to: 725-750
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deps/libscram/src/scram.c` around lines 682 - 683, The call to
scram_fast_hmac_create() must be NULL-checked before any subsequent HMAC calls;
update the sites in scram.c (including the one at the shown diff and the similar
sites around the 725–750 region) to verify the returned scram_fast_hmac_ctx* is
non-NULL immediately after scram_fast_hmac_create(), and if NULL avoid calling
scram_fast_hmac_init() or scram_fast_hmac_error() — instead propagate an
error/cleanup (return an error code or goto the existing cleanup path) and log
the failure using available logging routines so the OOM/OpenSSL setup failure is
handled safely.
| arg->connect_us = malloc(arg->iterations * sizeof(double)); | ||
| arg->close_us = malloc(arg->iterations * sizeof(double)); | ||
| arg->total_us = malloc(arg->iterations * sizeof(double)); |
There was a problem hiding this comment.
Unchecked heap allocations can crash the benchmark on OOM.
Lines 37-39, 125-126, 146-148, and 171-172 dereference allocation results without checks.
💡 Suggested fix
+static void *xmalloc(size_t n) {
+ void *p = malloc(n);
+ if (!p) {
+ fprintf(stderr, "Out of memory\n");
+ exit(1);
+ }
+ return p;
+}
+
+static void *xcalloc(size_t n, size_t sz) {
+ void *p = calloc(n, sz);
+ if (!p) {
+ fprintf(stderr, "Out of memory\n");
+ exit(1);
+ }
+ return p;
+}
...
- arg->connect_us = malloc(arg->iterations * sizeof(double));
- arg->close_us = malloc(arg->iterations * sizeof(double));
- arg->total_us = malloc(arg->iterations * sizeof(double));
+ arg->connect_us = xmalloc(arg->iterations * sizeof(double));
+ arg->close_us = xmalloc(arg->iterations * sizeof(double));
+ arg->total_us = xmalloc(arg->iterations * sizeof(double));
...
- pthread_t *tids = calloc(threads, sizeof(pthread_t));
- struct thread_arg *args = calloc(threads, sizeof(struct thread_arg));
+ pthread_t *tids = xcalloc(threads, sizeof(pthread_t));
+ struct thread_arg *args = xcalloc(threads, sizeof(struct thread_arg));
...
- double *connect_us = malloc(total_n * sizeof(double));
- double *close_us = malloc(total_n * sizeof(double));
- double *total_us = malloc(total_n * sizeof(double));
+ double *connect_us = xmalloc(total_n * sizeof(double));
+ double *close_us = xmalloc(total_n * sizeof(double));
+ double *total_us = xmalloc(total_n * sizeof(double));
...
- double *sorted_conn = malloc(n * sizeof(double));
- double *sorted_total = malloc(n * sizeof(double));
+ double *sorted_conn = xmalloc(n * sizeof(double));
+ double *sorted_total = xmalloc(n * sizeof(double));Also applies to: 125-126, 146-148, 171-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/bench_connect.c` around lines 37 - 39, The code performs multiple heap
allocations (e.g., arg->connect_us, arg->close_us, arg->total_us and other
mallocs at later sites) without checking for NULL; update each allocation site
to check the return value, handle OOM by freeing any earlier successful
allocations and returning an error (or exiting cleanly) instead of dereferencing
NULL, and ensure any caller (e.g., the function that allocates these arrays in
bench_connect.c) observes the failure so resources are not used; reference the
specific allocation targets arg->connect_us, arg->close_us, arg->total_us and
the other malloc results noted in the review and add proper NULL checks and
cleanup paths.
| for (int i = 1; i < argc; i++) { | ||
| if (strcmp(argv[i], "--host") == 0 && i+1 < argc) host = argv[++i]; | ||
| else if (strcmp(argv[i], "--port") == 0 && i+1 < argc) port = atoi(argv[++i]); | ||
| else if (strcmp(argv[i], "--user") == 0 && i+1 < argc) user = argv[++i]; | ||
| else if (strcmp(argv[i], "--password") == 0 && i+1 < argc) password = argv[++i]; | ||
| else if (strcmp(argv[i], "--dbname") == 0 && i+1 < argc) dbname = argv[++i]; | ||
| else if (strcmp(argv[i], "--ssl-ca") == 0 && i+1 < argc) ssl_ca = argv[++i]; | ||
| else if (strcmp(argv[i], "--iterations") == 0 && i+1 < argc) iterations = atoi(argv[++i]); | ||
| else if (strcmp(argv[i], "--warmup") == 0 && i+1 < argc) warmup = atoi(argv[++i]); | ||
| else if (strcmp(argv[i], "--label") == 0 && i+1 < argc) label = argv[++i]; | ||
| else if (strcmp(argv[i], "--threads") == 0 && i+1 < argc) threads = atoi(argv[++i]); | ||
| else { fprintf(stderr, "Unknown arg: %s\n", argv[i]); return 1; } | ||
| } | ||
|
|
||
| if (iterations > MAX_ITERS) { | ||
| fprintf(stderr, "Too many iterations (max %d)\n", MAX_ITERS); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Validate CLI numeric inputs and guard threads * iterations overflow.
Line 112 only caps the upper bound of iterations. Invalid values (e.g., --threads 0, negative values, or overflow at Line 145) can produce n<=0, divide-by-zero at Line 184, and invalid indexing at Lines 200-213.
💡 Suggested fix
+#include <limits.h>
...
- if (iterations > MAX_ITERS) {
- fprintf(stderr, "Too many iterations (max %d)\n", MAX_ITERS);
+ if (iterations <= 0 || iterations > MAX_ITERS || threads <= 0 || warmup < 0) {
+ fprintf(stderr, "Invalid args: iterations must be 1..%d, threads > 0, warmup >= 0\n", MAX_ITERS);
return 1;
}
+ if (threads > INT_MAX / iterations) {
+ fprintf(stderr, "threads * iterations overflow\n");
+ return 1;
+ }
...
- int total_n = threads * iterations;
+ int total_n = threads * iterations;Also applies to: 145-146, 184-184, 200-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/bench_connect.c` around lines 98 - 115, Validate and clamp all numeric
CLI inputs: ensure iterations is between 1 and MAX_ITERS, threads >= 1, warmup
>= 0 (and warmup <= iterations if intended), and port is in a valid range;
reject or error on invalid values (e.g., "--threads 0" or negative numbers).
Before computing any product like total_ops = threads * iterations (used later
to derive n and that can overflow), perform the multiplication in an unsigned
wider type (e.g., uint64_t/size_t) and explicitly check for overflow or that
total_ops > 0; ensure any derived n is > 0 before doing divisions to avoid
divide-by-zero. Return a non-zero error when validation fails and add clear
stderr messages referencing the offending flag (iterations, threads, warmup,
port) so downstream code (uses of n and indexing into per-thread arrays) never
sees invalid or overflowing sizes.
| pthread_create(&tids[t], NULL, thread_worker, &args[t]); | ||
| } | ||
| for (int t = 0; t < threads; t++) | ||
| pthread_join(tids[t], NULL); |
There was a problem hiding this comment.
Check pthread_create / pthread_join return codes.
Line 136 and Line 139 ignore failures; a failed create/join can leave invalid benchmark state and misleading results.
💡 Suggested fix
for (int t = 0; t < threads; t++) {
args[t].thread_id = t;
strncpy(args[t].conninfo, conninfo, sizeof(args[t].conninfo) - 1);
args[t].iterations = iterations;
args[t].warmup = warmup;
- pthread_create(&tids[t], NULL, thread_worker, &args[t]);
+ int rc = pthread_create(&tids[t], NULL, thread_worker, &args[t]);
+ if (rc != 0) {
+ fprintf(stderr, "pthread_create failed for thread %d (rc=%d)\n", t, rc);
+ return 1;
+ }
}
- for (int t = 0; t < threads; t++)
- pthread_join(tids[t], NULL);
+ for (int t = 0; t < threads; t++) {
+ int rc = pthread_join(tids[t], NULL);
+ if (rc != 0) {
+ fprintf(stderr, "pthread_join failed for thread %d (rc=%d)\n", t, rc);
+ return 1;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pthread_create(&tids[t], NULL, thread_worker, &args[t]); | |
| } | |
| for (int t = 0; t < threads; t++) | |
| pthread_join(tids[t], NULL); | |
| for (int t = 0; t < threads; t++) { | |
| args[t].thread_id = t; | |
| strncpy(args[t].conninfo, conninfo, sizeof(args[t].conninfo) - 1); | |
| args[t].iterations = iterations; | |
| args[t].warmup = warmup; | |
| int rc = pthread_create(&tids[t], NULL, thread_worker, &args[t]); | |
| if (rc != 0) { | |
| fprintf(stderr, "pthread_create failed for thread %d (rc=%d)\n", t, rc); | |
| return 1; | |
| } | |
| } | |
| for (int t = 0; t < threads; t++) { | |
| int rc = pthread_join(tids[t], NULL); | |
| if (rc != 0) { | |
| fprintf(stderr, "pthread_join failed for thread %d (rc=%d)\n", t, rc); | |
| return 1; | |
| } | |
| } |
🧰 Tools
🪛 Cppcheck (2.20.0)
[warning] 139-139: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/bench_connect.c` around lines 136 - 139, Check and handle return values
from pthread_create and pthread_join around the loop that spawns thread_worker
using tids, args and threads: capture each pthread_create return value, and on
non-zero log an error (including errno via strerror if desired), clean up any
allocated resources and exit or return a failure code instead of proceeding;
similarly capture each pthread_join return value and handle join failures (log
and abort/cleanup) to avoid inconsistent benchmark state. Ensure the error
handling occurs in the same scope where tids and args are used so threads are
not assumed to have started or completed when they have not.
| double *sorted_conn = malloc(n * sizeof(double)); | ||
| double *sorted_total = malloc(n * sizeof(double)); | ||
| memcpy(sorted_conn, connect_us, n * sizeof(double)); | ||
| memcpy(sorted_total, total_us, n * sizeof(double)); | ||
| qsort(sorted_conn, n, sizeof(double), cmp_double); | ||
| qsort(sorted_total, n, sizeof(double), cmp_double); |
There was a problem hiding this comment.
close_us min/p50 are computed from unsorted data.
Line 206/207 and Line 230 read close_us[0]/close_us[n/2] without sorting, so reported close min/p50 are incorrect.
💡 Suggested fix
- double *sorted_conn = malloc(n * sizeof(double));
- double *sorted_total = malloc(n * sizeof(double));
+ double *sorted_conn = malloc(n * sizeof(double));
+ double *sorted_close = malloc(n * sizeof(double));
+ double *sorted_total = malloc(n * sizeof(double));
memcpy(sorted_conn, connect_us, n * sizeof(double));
+ memcpy(sorted_close, close_us, n * sizeof(double));
memcpy(sorted_total, total_us, n * sizeof(double));
qsort(sorted_conn, n, sizeof(double), cmp_double);
+ qsort(sorted_close, n, sizeof(double), cmp_double);
qsort(sorted_total, n, sizeof(double), cmp_double);
...
- printf(" %-12s %10.1f %10.1f %10.1f\n",
- "Close", close_us[0], avg_close,
- close_us[n/2]);
+ printf(" %-12s %10.1f %10.1f %10.1f\n",
+ "Close", sorted_close[0], avg_close,
+ percentile(sorted_close, n, 0.50));
...
- fprintf(stderr, "\"close_us\":{\"min\":%.1f,\"avg\":%.1f,\"p50\":%.1f},",
- close_us[0], avg_close, close_us[n/2]);
+ fprintf(stderr, "\"close_us\":{\"min\":%.1f,\"avg\":%.1f,\"p50\":%.1f},",
+ sorted_close[0], avg_close, percentile(sorted_close, n, 0.50));
...
- free(sorted_conn); free(sorted_total);
+ free(sorted_conn); free(sorted_close); free(sorted_total);Also applies to: 205-207, 229-230, 241-241
🧰 Tools
🪛 Cppcheck (2.20.0)
[warning] 173-173: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 173-173: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 174-174: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 174-174: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 175-175: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 176-176: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/bench_connect.c` around lines 171 - 176, The min/p50 for close_us are
being read from the unsorted close_us array (uses close_us[0] and close_us[n/2])
which yields incorrect stats; follow the pattern used for connect_us/total_us by
allocating a new array (e.g., double *sorted_close), memcpy close_us into it,
qsort(sorted_close, n, sizeof(double), cmp_double), then use sorted_close[0] and
sorted_close[n/2] where the code currently reads close_us[0]/close_us[n/2];
remember to free sorted_close when done and keep naming consistent with
sorted_conn/sorted_total.
On OpenSSL 3.x, EVP_sha256() triggers EVP_MD_fetch() on every call, involving global lock contention and provider property lookups. The vendored PostgreSQL hmac_openssl.c calls it on every HMAC_Init_ex(), which means 4096 redundant fetches per SCRAM PBKDF2 derivation alone. This was responsible for ~58% of CPU time during SCRAM-heavy workloads, making ProxySQL 1.64x slower than PgBouncer at connect/disconnect. Two optimizations: 1. Cached EVP_MD* for HMAC (scram_hmac_cached.c): New scram_fast_* API that fetches the SHA-256 EVP_MD once via pthread_once and reuses it across all HMAC operations. Replaces all pg_hmac_* calls in libscram with the cached versions. No changes to vendored PostgreSQL code. 2. SCRAM verifier cache for plaintext passwords (scram.c): When ProxySQL stores plaintext passwords, every SCRAM exchange runs the full PBKDF2 (4096 HMAC iterations). This cache stores the generated SCRAM verifier after the first successful auth, so subsequent connections skip PBKDF2 entirely -- reducing HMAC operations from ~4096 to ~4 per connection, matching PgBouncer's behavior when a SCRAM verifier is stored directly. Thread-local, no locks needed. Result: ProxySQL connect/disconnect performance goes from 1.64x slower to parity with PgBouncer. Perf profile shifts from pg_hmac_init/EVP_MD_fetch (58%) to do_ssl_handshake (22%) -- the actual TLS work.
Generation counter for SCRAM verifier cache: - Global atomic counter incremented on pgsql_users reload - Thread-local caches check generation on lookup; stale entries are cleared and rebuilt from scratch - scram_cache_invalidate() called from __refresh_pgsql_users() - extern "C" guards added to scram_hmac_cached.h for C++ callers TAP test (pgsql-scram_cache_invalidation-t): - Creates user with pass1, populates SCRAM cache via connect loop - Changes password to pass2, reloads users (invalidates cache) - Verifies pass2 works and pass1 is rejected - Assigned to legacy-g4 test groups Benchmark tool (tools/bench_connect): - Multi-threaded SCRAM+TLS connect/disconnect benchmark - Uses vendored PostgreSQL from deps/ - Added to tools/Makefile with clean target
6d8dd5f to
e9637be
Compare
|
1 participant
Summary
On OpenSSL 3.x,
EVP_sha256()triggersEVP_MD_fetch()on every call, involving global lock contention and provider property lookups. The vendored PostgreSQLhmac_openssl.ccalls it on everyHMAC_Init_ex(), which means 4096 redundant fetches per SCRAM PBKDF2 derivation alone. This was responsible for ~58% of CPU time during SCRAM-heavy workloads.Two optimizations:
Cached EVP_MD* for HMAC (
scram_hmac_cached.c): Newscram_fast_*API that fetches the SHA-256EVP_MDonce viapthread_onceand reuses it across all HMAC operations. Replaces allpg_hmac_*calls in libscram with the cached versions. No changes to vendored PostgreSQL code.SCRAM verifier cache for plaintext passwords (
scram.c): When ProxySQL stores plaintext passwords, every SCRAM exchange runs the full PBKDF2 (4096 HMAC iterations). This cache stores the generated SCRAM verifier after the first successful auth, so subsequent connections skip PBKDF2 entirely — reducing HMAC operations from ~4096 to ~4 per connection, matching PgBouncer's behavior. Thread-local, no locks needed.Benchmark results
Serial connect/disconnect benchmark (10K iterations, C client via libpq, SCRAM-SHA-256 + TLS, 1 thread, no backend):
perfprofile shifts frompg_hmac_init/EVP_MD_fetch(58%) todo_ssl_handshake(22%) — the actual TLS work.Files changed
deps/libscram/include/scram_hmac_cached.h— new header withscram_fast_*APIdeps/libscram/src/scram_hmac_cached.c— cached EVP_MD HMAC implementation + fast SCRAM key derivationdeps/libscram/src/scram.c— switch toscram_fast_*functions, add SCRAM verifier cacheTest plan
perf recordconfirms HMAC/EVP_MD_fetch no longer dominatesscram_hmac_cached.cand modifiedscram.ccompile cleanly (gcc, OpenSSL 3.0.13)Summary by CodeRabbit