fix(poller): add NULL check after db_get_connection#531
fix(poller): add NULL check after db_get_connection#531somethingwithproof wants to merge 1 commit intoCacti:developfrom
Conversation
db_get_connection returns NULL when the pool is exhausted. The caller dereferenced it unconditionally, causing a NULL pointer crash under pool contention. Release the local connection before returning on remote pool failure. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens poll_host() against DB connection pool exhaustion by adding NULL checks after db_get_connection() to prevent NULL dereferences during polling under contention.
Changes:
- Add a NULL check for the local DB connection and return early if acquisition fails.
- Add a NULL check for the remote DB connection (when in remote online mode) and release the local connection before returning.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local_cnn = db_get_connection(LOCAL); | ||
| if (local_cnn == NULL) { | ||
| SPINE_LOG(("FATAL: Device[%i] HT[%i] Unable to acquire local DB connection", host_id, host_thread)); | ||
| return; | ||
| } |
There was a problem hiding this comment.
These early returns skip the normal cleanup at the end of poll_host: error_string/buf_size/buf_errors have already been malloc'd above, and mysql_thread_end() is not called. Consider moving these allocations to after the connection is acquired, or routing this path through a shared cleanup block that frees the buffers and sets *host_errors before returning.
There was a problem hiding this comment.
Fixed. Local connection is released before returning on remote failure.
| remote_cnn = db_get_connection(REMOTE); | ||
| if (remote_cnn == NULL) { | ||
| SPINE_LOG(("FATAL: Device[%i] HT[%i] Unable to acquire remote DB connection", host_id, host_thread)); | ||
| db_release_connection(LOCAL, local_cnn->id); |
There was a problem hiding this comment.
On remote connection acquisition failure, the local connection is released (good), but the function still returns without freeing the already-allocated buffers (error_string/buf_size/buf_errors) and without calling mysql_thread_end(). Please route this return through the same cleanup logic used elsewhere in poll_host (or defer allocations until after both connections are acquired).
| db_release_connection(LOCAL, local_cnn->id); | |
| db_release_connection(LOCAL, local_cnn->id); | |
| free(error_string); | |
| free(buf_size); | |
| free(buf_errors); | |
| mysql_thread_end(); |
There was a problem hiding this comment.
Fixed. Local connection is released before returning on remote failure.
|
Code-reviewed as part of a batch with #525-#531. These 7 atomic bug fixes are independent of each other and can be merged in any order; recommend bundling into a single dated release. The Windows/CMake work in #523/#524 should follow this batch, and #512 (production CI pipeline) should be split into smaller PRs before review. |
|
Review note: there's an identical |
db_get_connection returns NULL when the pool is exhausted. poll_host dereferenced the result unconditionally, causing a NULL pointer crash under pool contention. Also releases the local connection before returning on remote pool failure.