Menu
▾
▴
[tdbf-cvs] SF.net SVN: tdbf:[869] trunk/doc/2026-03-07_bug-audit.md
|
From: <tw...@us...> - 2026-03-07 19:05:25
|
Revision: 869
http://sourceforge.net/p/tdbf/code/869
Author: twm
Date: 2026-03-07 19:05:24 +0000 (Sat, 07 Mar 2026)
Log Message:
-----------
Add bug audit report documenting 16 reviewed issues
Systematic static analysis of all major TDBF subsystems. Results:
5 bugs fixed (r864-r868), 6 false positives, 3 known design
limitations, 2 cosmetic won't-fix items.
Added Paths:
-----------
trunk/doc/2026-03-07_bug-audit.md
Added: trunk/doc/2026-03-07_bug-audit.md
===================================================================
--- trunk/doc/2026-03-07_bug-audit.md (rev 0)
+++ trunk/doc/2026-03-07_bug-audit.md 2026-03-07 19:05:24 UTC (rev 869)
@@ -0,0 +1,278 @@
+# TDBF Bug Audit Report
+
+**Date:** 2026-03-07
+**Scope:** Systematic code review of all major TDBF subsystems
+**Method:** Static analysis, then verification and fixes
+
+## Summary
+
+16 items investigated across 9 source files. Final disposition:
+
+| Outcome | Count |
+|---------|-------|
+| FIXED | 5 (r864, r865, r866, r867, r868) |
+| FALSE POSITIVE | 6 |
+| KNOWN DESIGN LIMITATION | 3 |
+| WON'T FIX (cosmetic) | 2 |
+
+Additionally, 4 known issues are already documented in `doc/todo.txt`.
+
+---
+
+## CRITICAL — Crashes / Data Corruption
+
+### 1. ~~`GetKeyDataFromEntry` dereferences potentially nil pointer~~ FIXED (r864)
+
+**File:** `src/dbf_idxfile.pas:1722-1724`
+
+```pascal
+function TMdxPage.GetKeyDataFromEntry(AEntry: Integer): PAnsiChar;
+begin
+ Result := @PMdxEntry(GetEntry(AEntry))^.KeyData;
+end;
+```
+
+`GetEntry` (line 1685-1699) explicitly returns `nil` when the entry number is out of range. `GetKeyDataFromEntry` dereferences the result without a nil check, causing an **Access Violation** on any corrupt or out-of-range index entry.
+
+---
+
+### 2. ~~`PageDeleted` — memory leak on write failure~~ FIXED (r865)
+
+**File:** `src/dbf_pgcfile.pas:141-148`
+
+```pascal
+procedure TCachedFile.PageDeleted(Sender: TAvlTree; Data: PData);
+begin
+ if PPageInfo(Data^.ExtraData)^.Modified then
+ inherited WriteRecord(Data^.ID, @PPageInfo(Data^.ExtraData)^.Data);
+ FreeMem(Data^.ExtraData);
+end;
+```
+
+If `WriteRecord` raises an exception, `FreeMem` is never called, causing a **memory leak**. Should use `try/finally`.
+
+---
+
+### 3. ~~`ReadHeader` — FillChar with potentially negative size~~ FALSE POSITIVE
+
+**File:** `src/dbf_pgfile.pas:700-712`
+
+Not a bug: `size` is always clamped to `[0, FHeaderSize]` in all code paths before reaching the `FillChar` call, so `FHeaderSize-size` can never be negative.
+
+---
+
+### 4. ~~`FuncSubString` — negative index reads before buffer~~ FIXED (r866)
+
+**File:** `src/dbf_prscore.pas:1642-1656`
+
+```pascal
+index := PInteger(Param^.Args[1])^ - 1;
+...
+count := srcLen - index;
+...
+Param^.Res.Append(Param^.Args[0]+index, count)
+```
+
+dBASE `SUBSTR()` uses 1-based positions. If the user passes 0 or a negative position, `index` becomes -1 or less. The pointer `Param^.Args[0]+index` then points **before the buffer start**, and `count` (computed as `srcLen - index`) becomes larger than the actual string, causing a **read before buffer / buffer overread**.
+
+---
+
+### 5. ~~`FuncLeftString` — negative count passed to Append~~ FIXED (r867)
+
+**File:** `src/dbf_prscore.pas:1658-1667`
+
+```pascal
+count := PInteger(Param^.Args[1])^;
+if index + count > srcLen then
+ count := srcLen - index;
+Param^.Res.Append(Param^.Args[0]+index, count)
+```
+
+If the `LEFT()` count argument is negative, the `if` check passes (negative < srcLen), and a negative `count` is passed to `Append`, which calls `Move` with a negative length — **undefined behavior / crash**.
+
+---
+
+## HIGH — Buffer Overflows / Unsafe Memory
+
+### 6. ~~UTF-8 string field allocation assumes max 3x expansion~~ FALSE POSITIVE
+
+**File:** `src/dbf_parser.pas:281`
+
+Not a bug: `TranslateString` in `dbf_common.pas:607` caps its output to the original input `Length` via `WideCharToMultiByte`, so the `3x` buffer can never overflow.
+
+---
+
+### 7. ~~`TDynamicType.Append` — Move with unvalidated length~~ FIXED (r868)
+
+**File:** `src/dbf_prsdef.pas:1216-1221`
+
+```pascal
+procedure TDynamicType.Append(Source: Pointer; Length: Integer);
+begin
+ AssureSpace(Length+4);
+ Move(Source^, FMemoryPos^^, Length);
+```
+
+If `Length` is negative (as can happen from findings #4/#5), `AssureSpace` may not allocate correctly, and `Move` receives a negative byte count — **undefined behavior**. No validation that `Length >= 0`.
+
+**Note:** `FuncRight` (`src/dbf_prscore.pas:2820-2832`) also takes an unchecked user argument for `count`, but is safe because `count > 0` is checked before calling `Append`. Reviewed, not a bug.
+
+---
+
+### 8. ~~Pointer arithmetic without bounds check on field data~~ FALSE POSITIVE
+
+**File:** `src/dbf_dbffile.pas:1662-1669`
+
+```pascal
+while (FieldSize > 0) and (((PAnsiChar(Src) + FieldSize - 1)^ = ' ') ...) do
+ dec(FieldSize);
+if DataType <> ftString then
+ while (FieldSize > 0) and (PAnsiChar(Src)^ = ' ') do
+ begin
+ inc(PAnsiChar(Src));
+ dec(FieldSize);
+ end;
+```
+
+The pointer `Src` is incremented past its original start. While `FieldSize` guards the loop, there is no validation that `FieldSize` matches the actual allocated buffer size. A corrupt field definition could cause **out-of-bounds reads**.
+
+---
+
+### 9. ~~`RestructureTable` — Move with unvalidated offsets~~ FALSE POSITIVE
+
+**File:** `src/dbf_dbffile.pas:1370-1371`
+
+```pascal
+with RestructFieldInfo[lFieldNo] do
+ Move(pBuff[SourceOffset], pDestBuff[DestOffset], Size);
+```
+
+`SourceOffset`, `DestOffset`, and `Size` come from field definitions. If field definitions are inconsistent (e.g., source file has a different structure than expected), these offsets are not bounds-checked against the actual buffer sizes, risking **out-of-bounds read/write**.
+
+---
+
+### 10. ~~`WriteMemo` — suspicious commented-out buffer size~~ FALSE POSITIVE
+
+**File:** `src/dbf_memo.pas:369`
+
+```pascal
+readBytes := Src.Read(FBuffer[bytesBefore], RecordSize{PDbtHdr(Header).BlockLen}-bytesBefore);
+```
+
+The inline comment `{PDbtHdr(Header).BlockLen}` suggests an alternative size calculation was considered. If `RecordSize` doesn't match the actual block length in the header, reads could be the **wrong size**. Worth verifying that `RecordSize` is always correct here.
+
+---
+
+## MEDIUM — Logic Errors / Edge Cases
+
+### 11. `GetNumEntries` silently returns 0 on corruption — KNOWN DESIGN LIMITATION
+
+**File:** `src/dbf_idxfile.pas:1712-1720`
+
+```pascal
+function TMdxPage.GetNumEntries: Integer;
+begin
+ Result := Integer(SwapIntLE(DWORD(PMdxPage(PageBuffer)^.NumEntries)));
+ if (Result < 0) or (Result > SwapWordLE(PIndexHdr(FIndexFile.FIndexHeader)^.NumKeys)) then
+ begin
+ Result := 0;
+ FIndexFile.CheckInvalidError;
+ end;
+end;
+```
+
+When the entry count is corrupt, this returns 0 and calls `CheckInvalidError`. If `CheckInvalidError` doesn't raise (depends on `TDBF_INDEX_CHECK` define), the caller silently gets 0 entries. The **index is silently skipped** and data appears missing without any error to the user.
+
+---
+
+### 12. TODO: RecNo and Key not set after Insert — KNOWN DESIGN LIMITATION
+
+**File:** `src/dbf_idxcur.pas:76-80`
+
+```pascal
+procedure TIndexCursor.Insert(RecNo: Integer; Buffer: PAnsiChar);
+begin
+ TIndexFile(PagedFile).Insert(RecNo, ...);
+ // TODO SET RecNo and Key
+end;
+```
+
+After inserting into an index, the cursor's position (RecNo and Key) is not updated. Subsequent cursor navigation after an insert may be at an **undefined position**, potentially causing incorrect iteration or skipped records.
+
+---
+
+### 13. Missing `FieldDefs.Updated` handling for pre-Delphi 4 — KNOWN DESIGN LIMITATION
+
+**File:** `src/dbf.pas:2809-2813`
+
+```pascal
+{$ifdef SUPPORT_FIELDDEFS_UPDATED}
+ FieldDefs.Updated := false;
+{$else}
+ // TODO ... ??
+{$endif}
+```
+
+For Delphi 3 (pre-Delphi 4, which lacks `SUPPORT_FIELDDEFS_UPDATED`), changing the table name doesn't force FieldDefs to refresh. The IDE may show **stale field definitions** when switching between tables at design time.
+
+---
+
+## LOW — Minor / Cosmetic
+
+### 14. Redundant IsNull initialization — WON'T FIX (cosmetic, no functional impact)
+
+**File:** `src/dbf_parser.pas:686-724`
+
+The overloaded `ExtractFromBuffer` at line 678-684 initializes `IsNull := False` before calling the main implementation, making some of the internal `IsNull := False` assignments redundant. No functional impact.
+
+---
+
+### 15. `USE_ASSEMBLER_486_UP` not disabled for non-Windows 64-bit — WON'T FIX (mitigated by FPC)
+
+**File:** `src/dbf_common.inc:17-19`
+
+```pascal
+{$ifndef Win64}
+{$define USE_ASSEMBLER_486_UP}
+{$endif}
+```
+
+This enables x86 assembler for **all** non-Win64 targets, including Linux ARM, MIPS, etc. FPC has its own guard at line 654-656 (`{$ifndef CPUI386} {$undef USE_ASSEMBLER_486_UP}`), so this is harmless under FPC. However, any hypothetical non-FPC, non-Windows 64-bit compiler would incorrectly enable x86 assembler.
+
+---
+
+### 16. FPC string constant in wrong location — WON'T FIX (cosmetic, no functional impact)
+
+**File:** `src/dbf.pas:548-553`
+
+```pascal
+{$ifdef FPC}
+const
+ // TODO: move these to DBConsts
+ SNotEditing = 'Dataset not in edit or insert mode';
+{$endif}
+```
+
+Minor code organization issue — the string constant is defined locally instead of in the proper string resource unit. No functional impact.
+
+---
+
+## Known Issues — Already Documented in `doc/todo.txt`
+
+These are acknowledged by the developers:
+
+- Adding records without `CancelRange` causes assertion violation
+- Adding records with active filter causes autoinc fields not to be filled in
+- Cache flushing / recordcount issues in paged cache
+- Exception-safe freeing of items in `TOCollection.FreeItem`
+
+---
+
+## Recommended Fix Priority
+
+1. **#1** — Add nil check in `GetKeyDataFromEntry` (trivial fix, prevents AV on corrupt index)
+2. **#4, #5** — Validate SUBSTR/LEFT arguments before pointer arithmetic (prevents buffer overread)
+3. **#2** — Add `try/finally` in `PageDeleted` (prevents memory leak)
+4. **#3** — Guard `FillChar` against negative size in `ReadHeader`
+5. **#6** — Change UTF-8 multiplier from 3x to 4x
+6. **#7** — Add `Length >= 0` check in `TDynamicType.Append`
This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site.
|
