fix: resolve many tdb bugs#35006
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
Pull request overview
This PR targets correctness and robustness of the embedded TDB (B-tree) library, focusing on overflow-page handling, resource cleanup on failure paths, and portability fixes (alignment/endian/operator precedence).
Changes:
- Fix portability and correctness issues in low-level macros and allocators (e.g.,
TDB_GET_U24,TDB_FLAG_NO,tdbReallocheader sizing). - Strengthen pager/pcache reliability by cleaning up resources on error paths (journal/file open failures, encryption failures, cache growth failures, fetch/init failures).
- Refactor and harden B-tree overflow encode/decode logic (including new helper extraction and additional bounds/assert checks), plus propagate callback errors in table traversal.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/ci/count_assert.py | Updates CI allowlist for expected assert/ASSERT_CORE occurrences due to new/changed assertions. |
| source/libs/tdb/src/inc/tdbUtil.h | Moves include under header guard for safer include semantics. |
| source/libs/tdb/src/inc/tdbOs.h | Fixes macro operator precedence for TDB_O_RDWR. |
| source/libs/tdb/src/inc/tdbInt.h | Makes U24 reads unaligned-safe and fixes TDB_FLAG_NO precedence bug. |
| source/libs/tdb/src/db/tdbUtil.c | Uses size_t metadata for tdbRealloc/tdbFree for better portability. |
| source/libs/tdb/src/db/tdbTable.c | Removes dead variables, fixes allocator mismatch, and returns traversal callback errors. |
| source/libs/tdb/src/db/tdbPCache.c | Cleans up already-created pages on cache growth failure. |
| source/libs/tdb/src/db/tdbPager.c | Fixes multiple failure-path leaks, adds journal pgno validation, improves page release/unlock behavior, and reworks internal hashset. |
| source/libs/tdb/src/db/tdbPage.c | Adds overflow bounds checks, improves error handling paths, and fixes a leak in defragment failure. |
| source/libs/tdb/src/db/tdbDb.c | Fixes pager removal from hash chain by using pHashNext. |
| source/libs/tdb/src/db/tdbBtree.c | Refactors overflow payload decode/encode logic and adds/updates pgno validity assertions. |
Comments suppressed due to low confidence (1)
source/libs/tdb/src/db/tdbBtree.c:1456
- If
tdbRealloc(NULL, pBt->pageSize)fails, this branch releasesofpbut returnsret(which is still 0 here). This will silently report success even though encoding cannot proceed. Return an appropriate error code (e.g.,terrno/ OOM) instead ofretin this failure path.
// local buffer for cell
SCell *pBuf = tdbRealloc(NULL, pBt->pageSize);
if (pBuf == NULL) {
tdbPCacheRelease(pBt->pPager->pCache, ofp, pTxn);
return ret;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
== has higher precedence than &, so the macro evaluated as (flags) & ((flag) == 0) instead of ((flags) & (flag)) == 0.
…tion on add failure
…Set null in tdbPagerAbort
…in tdbRealloc, header guard order, variable shadowing
…t buffer on CBC_Encrypt failure in tdbEncryptPage\n- remove dead pKey variable and tdbFree(pKey) call in tdbTbOpen\n- use tdbOsFree instead of taosMemoryFree in tdbTbcOpen error path\n- release page lock on CBC_Decrypt failure in tdbPagerInitPage\n- propagate error from tdbTbPopFreePage in tdbPagerRemoveFreePage\n- add overflow array bounds check in tdbPageInsertCell\n- use portable %td format for ptrdiff_t in tdbPageCopy\n- return error on page lock init failure in tdbPageCreate\n- log pointer before freeing in tdbPageDropCell\n- add missing parentheses in TDB_O_RDWR macro\n- free aCellIdx on early return in tdbPageDefragment\n- remove stale commented-out debug logging
- tdbPagerOpen: free pager and close fd on all failure paths (fd invalid, file-id generation, getFileSize) - tdbPagerBegin: close jfd and remove journal file on hashset creation failure - tdbPagerAbort/tdbPagerRestore: validate pgno from journal against dbOrigSize to guard against corruption - tdbPagerFetchPage: release page refcount on init failure, uninitialized page, and pager mismatch error paths - tdbPagerFetchFreePage: release page refcount on init failure - Remove stale commented-out debug code
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@localvar 云上 Assert 开启了嘛 |
Description
Summary
This PR fixes a wide range of bugs found through AI code review of
source/libs/tdb/, including memory leaks, use-after-free, resource leaks on error paths, data corruption risks, and portability issues. No functional behavior changes — all fixes target correctness and robustness of existing code.Changes by category
Memory & Resource Leaks
tdbBtreeNext/tdbBtreePrevwith centralized key/val cleanup, fixing overflow key/val memory leakstdbOsFree(nottaosMemoryFree) for consistencyData Corruption & Logic Bugs
kLen - nLeftKey→bytes) and use-after-free wherepDecoder->pValpointed into released page memory; now pre-allocates val buffer and copies datapHashNextfor hash chain,pNextfor list chain) when removing pagerdbOrigSizeto guard against corruptionHashset (open-addressing)
Error Propagation
tdbTbPopFreePagePortability & Correctness
~(flags))inttosize_tto avoid signed overflow%tdformat specifier forptrdiff_t#include "tdbInt.h"inside header guardCleanup
#if 0blocksIssue(s)
Checklist
Please check the items in the checklist if applicable.