fix(poller): add NULL check after db_get_connection at poll entry#534
fix(poller): add NULL check after db_get_connection at poll entry#534somethingwithproof wants to merge 1 commit into
Conversation
db_get_connection() can return NULL after a connection pool error (sql.c:525). The existing code dereferences unconditionally at the poll function entry point, causing a segfault. Follow-up to Cacti#531 which fixed the same pattern at a different call site in poller.c. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds defensive handling in poll_host() to avoid NULL dereferences when the DB connection pool is exhausted/unavailable, aligning with the earlier fix in #531.
Changes:
- Add NULL check after
db_get_connection(LOCAL)and return early on failure. - Add NULL check after
db_get_connection(REMOTE)(when applicable) and return early on failure. - Attempt to release the local connection when remote acquisition fails.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| remote_cnn = db_get_connection(REMOTE); | ||
| if (remote_cnn == NULL) { | ||
| SPINE_LOG(("FATAL: Unable to obtain remote database connection in poller")); | ||
| db_release_connection(LOCAL, local_cnn); |
There was a problem hiding this comment.
db_release_connection() takes an integer pool id (see sql.h), but this call passes the pool_t* pointer. This will free the wrong pool slot (or crash) and can cause pool corruption. Pass local_cnn->id instead.
| db_release_connection(LOCAL, local_cnn); | |
| db_release_connection(local_cnn->id, local_cnn); |
| local_cnn = db_get_connection(LOCAL); | ||
| if (local_cnn == NULL) { | ||
| SPINE_LOG(("FATAL: Unable to obtain local database connection in poller")); | ||
| return; | ||
| } |
There was a problem hiding this comment.
In this early-return path, error_string/buf_size/buf_errors have already been malloc()'d above, but they are not freed before returning. This will leak memory each time the pool is exhausted. Free these allocations (e.g., via SPINE_FREE) before returning.
| if (set.poller_id > 1 && set.mode == REMOTE_ONLINE) { | ||
| remote_cnn = db_get_connection(REMOTE); | ||
| if (remote_cnn == NULL) { | ||
| SPINE_LOG(("FATAL: Unable to obtain remote database connection in poller")); |
There was a problem hiding this comment.
On remote connection failure, the function returns after releasing the local DB connection, but it still leaks the previously allocated error_string/buf_size/buf_errors from above. Free those allocations before returning here as well.
| SPINE_LOG(("FATAL: Unable to obtain remote database connection in poller")); | |
| SPINE_LOG(("FATAL: Unable to obtain remote database connection in poller")); | |
| if (error_string != NULL) { | |
| free(error_string); | |
| } | |
| if (buf_size != NULL) { | |
| free(buf_size); | |
| } | |
| if (buf_errors != NULL) { | |
| free(buf_errors); | |
| } |
Follow-up to #531 which fixed the same NULL-dereference pattern at a different call site in poller.c.
Problem
db_get_connection(LOCAL)at poller.c:235 can return NULL after a connection pool error (sql.c:525). The code immediately dereferenceslocal_cnn->mysqlwithout checking, causing a segfault when the pool is exhausted or the database is unreachable.The same pattern exists for the
db_get_connection(REMOTE)call at line 239.Fix
Add NULL checks for both LOCAL and REMOTE connections before dereferencing. On LOCAL failure, return immediately. On REMOTE failure, release the LOCAL connection before returning to avoid a pool leak.
Identified during code review of the spine PR batch (#525-#532).