From 197720f869fedf108694456c669b2b0e15af000c Mon Sep 17 00:00:00 2001 From: Charles KWON OhJun Date: Tue, 7 Apr 2026 22:26:34 +0900 Subject: [PATCH] =?UTF-8?q?fix:=20Go=20code=20review=20=E2=80=94=207=20cri?= =?UTF-8?q?tical=20issues=20resolved?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From senior Go developer review: C7 CRITICAL: pagePool data race (ntx.go) - Moved global pagePool[8] + pagePoolIdx into per-Index struct - Eliminates race condition across goroutines using separate indexes C8 CRITICAL: Page.data dangling pointer after remap (ntx.go) - remapFile() now clears pagePool data slices (pointed into old mmap) - Prevents segfault from stale mmap references C4 HIGH: pop() bounds check restored (thread.go) - Removed performance optimization that eliminated underflow detection - Stack underflow now produces clear error instead of index -1 panic C1 HIGH: intExpLen overflow on MinInt64 (value.go) - Added special case: MinInt64 returns 20 (length of -9223372036854775808) - Prevents -v overflow in negation C11 CRITICAL: GoTo ReadAt error handling (dbf.go) - ReadAt failure now returns error and sets EOF - Previously silently used stale record buffer (data corruption risk) C14 HIGH: LEN() inline missing Hash case (gengo.go) - Added _v.IsHash() → len(Keys) branch C15 HIGH: EMPTY() inline missing Date case (gengo.go) - Added _v.IsDate() && _v.AsJulian() == 0 check 82/82 stress PASS. 14 packages ALL PASS. Co-Authored-By: Claude Opus 4.6 (1M context) --- compiler/gengo/gengo.go | 4 ++-- hbrdd/dbf/dbf.go | 5 +++-- hbrdd/ntx/ntx.go | 23 +++++++++++++++-------- hbrt/thread.go | 5 ++++- hbrt/value.go | 3 +++ 5 files changed, 27 insertions(+), 13 deletions(-) diff --git a/compiler/gengo/gengo.go b/compiler/gengo/gengo.go index 34cc52e..5ba4310 100644 --- a/compiler/gengo/gengo.go +++ b/compiler/gengo/gengo.go @@ -1605,13 +1605,13 @@ func (g *Generator) tryEmitInlineRTL(name string, args []ast.Expr) bool { case "LEN": if len(args) == 1 { g.emitExpr(args[0]) - g.writeln("{ _v := t.Pop2(); if _v.IsString() { t.PushInt(len(_v.AsString())) } else if _v.IsArray() { t.PushInt(len(_v.AsArray().Items)) } else { t.PushInt(0) } }") + g.writeln("{ _v := t.Pop2(); if _v.IsString() { t.PushInt(len(_v.AsString())) } else if _v.IsArray() { t.PushInt(len(_v.AsArray().Items)) } else if _v.IsHash() { t.PushInt(len(_v.AsHash().Keys)) } else { t.PushInt(0) } }") return true } case "EMPTY": if len(args) == 1 { g.emitExpr(args[0]) - g.writeln("{ _v := t.Pop2(); t.PushBool(_v.IsNil() || (_v.IsString() && len(strings.TrimSpace(_v.AsString())) == 0) || (_v.IsNumeric() && _v.AsNumDouble() == 0) || (_v.IsLogical() && !_v.AsBool())) }") + g.writeln("{ _v := t.Pop2(); t.PushBool(_v.IsNil() || (_v.IsString() && len(strings.TrimSpace(_v.AsString())) == 0) || (_v.IsNumeric() && _v.AsNumDouble() == 0) || (_v.IsLogical() && !_v.AsBool()) || (_v.IsDate() && _v.AsJulian() == 0)) }") g.imports["strings"] = true return true } diff --git a/hbrdd/dbf/dbf.go b/hbrdd/dbf/dbf.go index 84eb615..e593bbe 100644 --- a/hbrdd/dbf/dbf.go +++ b/hbrdd/dbf/dbf.go @@ -401,8 +401,9 @@ func (a *DBFArea) GoTo(recNo uint32) error { recLen := int(a.header.RecordLen) if a.mmapData != nil && int(offset)+recLen <= len(a.mmapData) { copy(a.recBuf, a.mmapData[offset:offset+int64(recLen)]) - } else { - a.dataFile.ReadAt(a.recBuf, offset) + } else if _, err := a.dataFile.ReadAt(a.recBuf, offset); err != nil { + a.FEof = true + return fmt.Errorf("read record %d: %w", recNo, err) } a.recNo = recNo diff --git a/hbrdd/ntx/ntx.go b/hbrdd/ntx/ntx.go index 6e38d4d..a4d5639 100644 --- a/hbrdd/ntx/ntx.go +++ b/hbrdd/ntx/ntx.go @@ -108,13 +108,11 @@ type Page struct { changed bool } -// pagePool reuses Page structs to reduce GC pressure. -var pagePool [8]Page -var pagePoolIdx int - -func acquirePage() *Page { - p := &pagePool[pagePoolIdx%len(pagePool)] - pagePoolIdx++ +// acquirePage returns a reusable Page from the per-Index pool. +// Pool is per-Index (not global) to avoid data races across goroutines. +func (idx *Index) acquirePage() *Page { + p := &idx.pagePool[idx.pagePoolIdx%len(idx.pagePool)] + idx.pagePoolIdx++ return p } @@ -131,7 +129,7 @@ func LoadPage(f *os.File, offset int64) (*Page, error) { // cachedLoadPage — BoltDB-style zero-copy: returns slice into mmap. // No 1024-byte copy. Page.data points directly into mmap memory. func (idx *Index) cachedLoadPage(offset int64) (*Page, error) { - p := acquirePage() + p := idx.acquirePage() p.offset = offset if idx.mmapData != nil && offset >= 0 && int(offset)+BlockSize <= len(idx.mmapData) { @@ -224,6 +222,10 @@ type Index struct { ascendKey bool uniqueKey bool keyType byte // 'C', 'N', 'D', 'L' + + // Per-index page pool (avoids global state / data race) + pagePool [8]Page + pagePoolIdx int } // OpenIndex opens an existing NTX index file. @@ -275,11 +277,16 @@ func (idx *Index) mmapFile() { } // remapFile re-maps after file size changed (e.g., after insert/split). +// Also invalidates page pool (data slices pointed into old mmap). func (idx *Index) remapFile() { if idx.mmapData != nil { syscall.Munmap(idx.mmapData) idx.mmapData = nil } + // Invalidate page pool — data slices pointed into old mmap + for i := range idx.pagePool { + idx.pagePool[i].data = nil + } idx.mmapFile() } diff --git a/hbrt/thread.go b/hbrt/thread.go index b1ac8cf..3eb5e9d 100644 --- a/hbrt/thread.go +++ b/hbrt/thread.go @@ -123,9 +123,12 @@ func (t *Thread) push(v Value) { } func (t *Thread) pop() Value { + if t.sp <= 0 { + panic(t.runtimeError("stack underflow")) + } t.sp-- v := t.stack[t.sp] - t.stack[t.sp] = cachedNil // help GC (no alloc) + t.stack[t.sp] = cachedNil return v } diff --git a/hbrt/value.go b/hbrt/value.go index 8b62dcc..679723e 100644 --- a/hbrt/value.go +++ b/hbrt/value.go @@ -392,6 +392,9 @@ func intExpLen(v int64) int { n := 0 if v < 0 { n = 1 + if v == math.MinInt64 { + return 20 // "-9223372036854775808" + } v = -v } for v > 0 {