Skip to content

Commit c995a1c

Browse files
mpywclaude
andauthored
fix: Handle multiple stores to same address for pointer-based access (#16)
Previously, findStoredValue only returned the first Store instruction found for a given address. This caused false negatives when multiple stores existed in different control flow paths (e.g., if/else branches). Changes: - Renamed findStoredValue to findAllStoredValues to return all stores - Added traceAllStoredValues to check ALL stored values have context (similar to Phi node semantics - all paths must have ctx) - Updated traceUnOp and traceAlloc to use the new functions Example that was previously missed: e := logger.Info().Ctx(ctx) ptr := &e if cond { *ptr = logger.Warn() // no ctx in this branch! } (*ptr).Msg("msg") // Now correctly reports missing ctx 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent cf6771a commit c995a1c

File tree

2 files changed

+66
-8
lines changed

2 files changed

+66
-8
lines changed

internal/ssa/tracing.go

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -373,21 +373,36 @@ func edgeLeadsToImpl(v ssa.Value, target *ssa.Phi, seen map[ssa.Value]bool) bool
373373
// traceUnOp handles SSA unary operations, especially pointer dereferences.
374374
func (c *Checker) traceUnOp(unop *ssa.UnOp, visited map[ssa.Value]bool, t tracerType) bool {
375375
if unop.Op == token.MUL {
376-
if stored := findStoredValue(unop.X); stored != nil {
377-
return c.traceValue(stored, t, visited)
376+
storedValues := findAllStoredValues(unop.X)
377+
if len(storedValues) > 0 {
378+
return c.traceAllStoredValues(storedValues, visited, t)
378379
}
379380
}
380381
return c.traceValue(unop.X, t, visited)
381382
}
382383

383384
// traceAlloc handles SSA Alloc nodes (local variable allocation).
384385
func (c *Checker) traceAlloc(alloc *ssa.Alloc, visited map[ssa.Value]bool, t tracerType) bool {
385-
if stored := findStoredValue(alloc); stored != nil {
386-
return c.traceValue(stored, t, visited)
386+
storedValues := findAllStoredValues(alloc)
387+
if len(storedValues) > 0 {
388+
return c.traceAllStoredValues(storedValues, visited, t)
387389
}
388390
return false
389391
}
390392

393+
// traceAllStoredValues traces all stored values and returns true only if ALL have context.
394+
// This is similar to Phi node handling - all paths must have context.
395+
func (c *Checker) traceAllStoredValues(storedValues []ssa.Value, visited map[ssa.Value]bool, t tracerType) bool {
396+
for _, stored := range storedValues {
397+
// Clone visited for independent tracing of each store
398+
storeVisited := maps.Clone(visited)
399+
if !c.traceValue(stored, t, storeVisited) {
400+
return false
401+
}
402+
}
403+
return true
404+
}
405+
391406
// traceFreeVar traces a FreeVar back to the value bound in MakeClosure.
392407
func (c *Checker) traceFreeVar(fv *ssa.FreeVar, visited map[ssa.Value]bool, t tracerType) bool {
393408
fn := fv.Parent()
@@ -476,8 +491,17 @@ func (c *Checker) traceIIFEReturns(fn *ssa.Function, visited map[ssa.Value]bool,
476491
// Store Tracking
477492
// =============================================================================
478493

479-
// findStoredValue finds the value that was stored at the given address.
480-
func findStoredValue(addr ssa.Value) ssa.Value {
494+
// findAllStoredValues finds all values that were stored at the given address.
495+
// Multiple stores can occur in different control flow paths (e.g., if/else branches).
496+
// All stored values must be checked for context to handle cases like:
497+
//
498+
// e := logger.Info().Ctx(ctx)
499+
// ptr := &e
500+
// if cond {
501+
// *ptr = logger.Warn() // no ctx in this branch!
502+
// }
503+
// (*ptr).Msg("msg") // should report: one branch lacks ctx
504+
func findAllStoredValues(addr ssa.Value) []ssa.Value {
481505
var fn *ssa.Function
482506
switch v := addr.(type) {
483507
case *ssa.FieldAddr:
@@ -495,18 +519,19 @@ func findStoredValue(addr ssa.Value) ssa.Value {
495519
return nil
496520
}
497521

522+
var storedValues []ssa.Value
498523
for _, block := range fn.Blocks {
499524
for _, instr := range block.Instrs {
500525
store, ok := instr.(*ssa.Store)
501526
if !ok {
502527
continue
503528
}
504529
if addressesMatch(store.Addr, addr) {
505-
return store.Val
530+
storedValues = append(storedValues, store.Val)
506531
}
507532
}
508533
}
509-
return nil
534+
return storedValues
510535
}
511536

512537
// addressesMatch checks if two addresses refer to the same memory location.

testdata/src/zerolog/evil_ssa.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1062,3 +1062,36 @@ func badLoggerFromContextThenNew(ctx context.Context, logger zerolog.Logger) {
10621062
_ = zerolog.Ctx(ctx) // Get but don't use
10631063
logger.Info().Msg("ignored ctx logger") // want `zerolog call chain missing .Ctx\(ctx\)`
10641064
}
1065+
1066+
// ===== POINTER WITH CONDITIONAL STORE =====
1067+
1068+
// Test case: Multiple stores to same pointer address - partial ctx
1069+
// If only one branch has ctx, should report
1070+
func badPointerConditionalStorePartialCtx(ctx context.Context, logger zerolog.Logger, cond bool) {
1071+
e := logger.Info() // no ctx
1072+
ptr := &e
1073+
if cond {
1074+
*ptr = logger.Warn().Ctx(ctx) // has ctx
1075+
}
1076+
(*ptr).Msg("ptr conditional partial") // want `zerolog call chain missing .Ctx\(ctx\)`
1077+
}
1078+
1079+
// Test case: Both stores have ctx - should be OK
1080+
func goodPointerConditionalStoreAllCtx(ctx context.Context, logger zerolog.Logger, cond bool) {
1081+
e := logger.Info().Ctx(ctx) // has ctx
1082+
ptr := &e
1083+
if cond {
1084+
*ptr = logger.Warn().Ctx(ctx) // has ctx
1085+
}
1086+
(*ptr).Msg("ptr conditional all ctx") // OK - all paths have ctx
1087+
}
1088+
1089+
// Test case: Initial has ctx, conditional does not
1090+
func badPointerConditionalStoreInitialCtx(ctx context.Context, logger zerolog.Logger, cond bool) {
1091+
e := logger.Info().Ctx(ctx) // has ctx
1092+
ptr := &e
1093+
if cond {
1094+
*ptr = logger.Warn() // no ctx
1095+
}
1096+
(*ptr).Msg("ptr initial ctx") // want `zerolog call chain missing .Ctx\(ctx\)`
1097+
}

0 commit comments

Comments
 (0)