fix(secmem-lint): match receivers and params by object, not by name

secmem-lint/analyzer.go

@@ -54,10 +54,15 @@ func run(pass *analysis.Pass) (any, error) {
 
 // accessor is a recognized secmem borrowing-closure call:
 // recv.Method(func(p []byte) { ... }).
+//
+// Both fields identify things by types.Object rather than by name or AST shape.
+// Names are the wrong key twice over: a receiver written as a field selector
+// (s.buf) is not an identifier at all, and a borrowed parameter's name can be
+// shadowed by an unrelated variable that happens to match.
 type accessor struct {
-	recv   *ast.Ident      // receiver identifier, or nil if not a plain identifier
-	fn     *ast.FuncLit    // the borrowing closure
-	params map[string]bool // names of its []byte parameters (the borrowed slices)
+	recv   []types.Object        // receiver identity chain, nil if undecidable
+	fn     *ast.FuncLit          // the borrowing closure
+	params map[types.Object]bool // its []byte parameters (the borrowed slices)
 }
 
 // borrowAccessor reports whether call is a secmem borrowing accessor and, if so,
@@ -87,14 +92,68 @@ func borrowAccessor(pass *analysis.Pass, call *ast.CallExpr) (accessor, bool) {
 	if lit == nil {
 		return accessor{}, false
 	}
-	params := byteSliceParams(lit)
+	params := byteSliceParams(pass, lit)
 	if len(params) == 0 {
 		return accessor{}, false
 	}
-	recv, _ := sel.X.(*ast.Ident)
+	recv, _ := receiverKey(pass, sel.X)
 	return accessor{recv: recv, fn: lit, params: params}, true
 }
 
+// receiverKey builds a comparison key for a receiver expression: the chain of
+// objects from the root identifier through any field selections, so buf and
+// s.buf and s.inner.buf each get a key that can be compared for identity.
+//
+// It reports false for shapes whose identity cannot be decided statically —
+// index expressions, calls, type assertions. bufs[i] and bufs[j] are written
+// alike and need not be the same buffer, and getBuf() twice need not return the
+// same one, so treating them as equal would be a false positive on a linter
+// whose findings block a build.
+func receiverKey(pass *analysis.Pass, expr ast.Expr) ([]types.Object, bool) {
+	switch e := expr.(type) {
+	case *ast.Ident:
+		obj := pass.TypesInfo.ObjectOf(e)
+		if obj == nil {
+			return nil, false
+		}
+		return []types.Object{obj}, true
+
+	case *ast.ParenExpr:
+		return receiverKey(pass, e.X)
+
+	case *ast.StarExpr:
+		// (*p).WithBytes(...) names the same buffer as p.WithBytes(...).
+		return receiverKey(pass, e.X)
+
+	case *ast.SelectorExpr:
+		base, ok := receiverKey(pass, e.X)
+		if !ok {
+			return nil, false
+		}
+		field := pass.TypesInfo.ObjectOf(e.Sel)
+		if field == nil {
+			return nil, false
+		}
+		key := make([]types.Object, 0, len(base)+1)
+		key = append(key, base...)
+		return append(key, field), true
+	}
+	return nil, false
+}
+
+// sameReceiver reports whether two receiver keys name the same buffer.
+func sameReceiver(a, b []types.Object) bool {
+	if len(a) == 0 || len(a) != len(b) {
+		return false
+	}
+	for i := range a {
+		if a[i] != b[i] {
+			return false
+		}
+	}
+	return true
+}
+
 func isBorrowMethod(pkgPath, method string) bool {
 	switch pkgPath {
 	case secmemPkg:
@@ -105,24 +164,27 @@ func isBorrowMethod(pkgPath, method string) bool {
 	return false
 }
 
-// byteSliceParams returns the names of the closure's []byte parameters — the
-// borrowed slices whose escape the checks track.
-func byteSliceParams(fn *ast.FuncLit) map[string]bool {
-	names := make(map[string]bool)
+// byteSliceParams returns the closure's []byte parameters — the borrowed slices
+// whose escape the checks track.
+func byteSliceParams(pass *analysis.Pass, fn *ast.FuncLit) map[types.Object]bool {
+	objs := make(map[types.Object]bool)
 	if fn.Type == nil || fn.Type.Params == nil {
-		return names
+		return objs
 	}
 	for _, field := range fn.Type.Params.List {
 		if !isByteSlice(field.Type) {
 			continue
 		}
 		for _, n := range field.Names {
-			if n.Name != "_" {
-				names[n.Name] = true
+			if n.Name == "_" {
+				continue
+			}
+			if obj := pass.TypesInfo.ObjectOf(n); obj != nil {
+				objs[obj] = true
 			}
 		}
 	}
-	return names
+	return objs
 }
 
 func isByteSlice(expr ast.Expr) bool {
@@ -196,26 +258,31 @@ func nolintApplies(comment string) bool {
 // refersToParam reports whether expr is (a paren/slice around) a borrowed param.
 // It deliberately does not recurse into calls, so len(p) or f(p) is not itself a
 // direct reference to the borrowed slice.
-func refersToParam(expr ast.Expr, params map[string]bool) bool {
+func refersToParam(pass *analysis.Pass, expr ast.Expr, params map[types.Object]bool) bool {
 	switch e := expr.(type) {
 	case *ast.Ident:
-		return params[e.Name]
+		return params[pass.TypesInfo.ObjectOf(e)]
 	case *ast.ParenExpr:
-		return refersToParam(e.X, params)
+		return refersToParam(pass, e.X, params)
 	case *ast.SliceExpr:
-		return refersToParam(e.X, params)
+		return refersToParam(pass, e.X, params)
 	}
 	return false
 }
 
 // goStmtLeaksParam reports whether a go statement hands a borrowed param to the
 // new goroutine — captured by a closure body or passed as an argument.
-func goStmtLeaksParam(call *ast.CallExpr, params map[string]bool) bool {
+//
+// The capture scan resolves each identifier to its object rather than comparing
+// names. A goroutine that declares its own b, or ranges over one, is not
+// touching the borrowed slice at all, and matching on the name alone reported it
+// as a leak.
+func goStmtLeaksParam(pass *analysis.Pass, call *ast.CallExpr, params map[types.Object]bool) bool {
 	if call == nil {
 		return false
 	}
 	for _, arg := range call.Args {
-		if refersToParam(arg, params) {
+		if refersToParam(pass, arg, params) {
 			return true
 		}
 	}
@@ -228,7 +295,7 @@ func goStmtLeaksParam(call *ast.CallExpr, params map[string]bool) bool {
 		if leaked {
 			return false
 		}
-		if id, ok := n.(*ast.Ident); ok && params[id.Name] {
+		if id, ok := n.(*ast.Ident); ok && params[pass.TypesInfo.ObjectOf(id)] {
 			leaked = true
 		}
 		return !leaked

secmem-lint/escape.go

@@ -17,11 +17,11 @@ func checkCallbackEscapes(pass *analysis.Pass, acc accessor, sup *suppressor) {
 		case *ast.CallExpr:
 			checkCallEscape(pass, acc, node, sup)
 		case *ast.SendStmt:
-			if refersToParam(node.Value, acc.params) && !sup.suppressed(pass, node.Pos()) {
+			if refersToParam(pass, node.Value, acc.params) && !sup.suppressed(pass, node.Pos()) {
 				report(pass, node.Pos(), "borrowed secret bytes sent to a channel; they can outlive the closure")
 			}
 		case *ast.GoStmt:
-			if goStmtLeaksParam(node.Call, acc.params) && !sup.suppressed(pass, node.Pos()) {
+			if goStmtLeaksParam(pass, node.Call, acc.params) && !sup.suppressed(pass, node.Pos()) {
 				report(pass, node.Pos(), "borrowed secret bytes handed to a goroutine; they can outlive the closure")
 			}
 		case *ast.AssignStmt:
@@ -37,7 +37,7 @@ func checkCallEscape(pass *analysis.Pass, acc accessor, call *ast.CallExpr, sup
 	if id, ok := call.Fun.(*ast.Ident); ok {
 		switch id.Name {
 		case "string":
-			if len(call.Args) == 1 && refersToParam(call.Args[0], acc.params) && !sup.suppressed(pass, call.Pos()) {
+			if len(call.Args) == 1 && refersToParam(pass, call.Args[0], acc.params) && !sup.suppressed(pass, call.Pos()) {
 				report(pass, call.Pos(), "string() copies borrowed secret bytes into a heap string")
 			}
 			return
@@ -46,20 +46,20 @@ func checkCallEscape(pass *analysis.Pass, acc accessor, call *ast.CallExpr, sup
 			// the same escape written with a slice expression, and matching
 			// only the identifier let it through.
 			if call.Ellipsis.IsValid() && len(call.Args) >= 2 {
-				if refersToParam(call.Args[len(call.Args)-1], acc.params) && !sup.suppressed(pass, call.Pos()) {
+				if refersToParam(pass, call.Args[len(call.Args)-1], acc.params) && !sup.suppressed(pass, call.Pos()) {
 					report(pass, call.Pos(), "append(dst, borrowed...) copies borrowed secret bytes into an escaping slice")
 				}
 			}
 			return
 		case "panic":
 			// The value is formatted into the runtime traceback and handed to
 			// any recover() up the stack, both well outside the lease.
-			if len(call.Args) == 1 && refersToParam(call.Args[0], acc.params) && !sup.suppressed(pass, call.Pos()) {
+			if len(call.Args) == 1 && refersToParam(pass, call.Args[0], acc.params) && !sup.suppressed(pass, call.Pos()) {
 				report(pass, call.Pos(), "panic() puts borrowed secret bytes in the traceback and in any recover()")
 			}
 			return
 		case "copy":
-			if len(call.Args) == 2 && refersToParam(call.Args[1], acc.params) && !sup.suppressed(pass, call.Pos()) {
+			if len(call.Args) == 2 && refersToParam(pass, call.Args[1], acc.params) && !sup.suppressed(pass, call.Pos()) {
 				report(pass, call.Pos(), "copy() moves borrowed secret bytes out of the closure")
 			}
 			return
@@ -76,7 +76,7 @@ func checkAssignEscape(pass *analysis.Pass, acc accessor, stmt *ast.AssignStmt,
 		return
 	}
 	for i, rhs := range stmt.Rhs {
-		if i >= len(stmt.Lhs) || !refersToParam(rhs, acc.params) {
+		if i >= len(stmt.Lhs) || !refersToParam(pass, rhs, acc.params) {
 			continue
 		}
 		where, escapes := assignTargetEscapes(pass, stmt.Lhs[i], acc)
@@ -183,7 +183,7 @@ func checkSink(pass *analysis.Pass, acc accessor, call *ast.CallExpr, sup *suppr
 		return
 	}
 	for _, arg := range call.Args {
-		if refersToParam(arg, acc.params) {
+		if refersToParam(pass, arg, acc.params) {
 			if !sup.suppressed(pass, call.Pos()) {
 				report(pass, call.Pos(), fmt.Sprintf("borrowed secret bytes passed to %s; %s", name, reason))
 			}

secmem-lint/reentrancy.go

@@ -34,12 +34,13 @@ var reentrantUnsafe = map[string]bool{ //nolint:gochecknoglobals // immutable lo
 // checkReentrancy flags an access method called on the SAME buffer inside its own
 // borrowing closure. A DIFFERENT buffer (the documented decrypt-into pattern) is
 // resolved by object identity and is not flagged.
+//
+// Receivers are compared as identity chains, so a buffer held in a struct field
+// is covered. Requiring a plain identifier on both ends — which is all this
+// check used to do — silently disabled it for s.buf.WithBytes(...), and holding
+// the buffer in a struct is how most programs of any size hold it.
 func checkReentrancy(pass *analysis.Pass, acc accessor, sup *suppressor) {
-	if acc.recv == nil {
-		return
-	}
-	recvObj := pass.TypesInfo.ObjectOf(acc.recv)
-	if recvObj == nil {
+	if len(acc.recv) == 0 {
 		return
 	}
 	ast.Inspect(acc.fn.Body, func(n ast.Node) bool {
@@ -51,8 +52,8 @@ func checkReentrancy(pass *analysis.Pass, acc accessor, sup *suppressor) {
 		if !ok || !reentrantUnsafe[sel.Sel.Name] {
 			return true
 		}
-		inner, ok := sel.X.(*ast.Ident)
-		if !ok || pass.TypesInfo.ObjectOf(inner) != recvObj {
+		inner, ok := receiverKey(pass, sel.X)
+		if !ok || !sameReceiver(acc.recv, inner) {
 			return true
 		}
 		if !sup.suppressed(pass, call.Pos()) {

secmem-lint/testdata/src/escape/escape.go

@@ -83,3 +83,17 @@ func cleanInnerAggregate(buf *secmem.SecureBuffer) {
 		_ = local
 	})
 }
+
+// shadowedNameInGoroutineOK: the goroutine declares its own b. The capture scan
+// used to match on the identifier's NAME, so this unrelated variable was
+// reported as a leak of the borrowed slice.
+func shadowedNameInGoroutineOK(buf *secmem.SecureBuffer, other [][]byte) {
+	_ = buf.WithBytes(func(b []byte) {
+		_ = b
+		go func() {
+			for _, b := range other {
+				_ = len(b)
+			}
+		}()
+	})
+}

secmem-lint/testdata/src/reentrancy/reentrancy.go

@@ -44,3 +44,44 @@ func sameBufferLockingInspectors(buf *secmem.SecureBuffer) {
 		_ = buf.IsDestroyed()   // want `secmem-lint: IsDestroyed called on the same buffer`
 	})
 }
+
+// vault holds its buffer in a struct field, which is how any program larger
+// than an example holds it. The check used to require a plain identifier on
+// both ends, so every one of these was silently clean.
+type vault struct {
+	buf   *secmem.SecureBuffer
+	inner struct{ buf *secmem.SecureBuffer }
+}
+
+func (v *vault) fieldReceiver() {
+	_ = v.buf.WithBytes(func(b []byte) {
+		_ = b
+		_ = v.buf.Len() // want `secmem-lint: Len called on the same buffer`
+	})
+}
+
+func (v *vault) nestedFieldReceiver() {
+	_ = v.inner.buf.WithBytes(func(b []byte) {
+		_ = b
+		_ = v.inner.buf.IsSealed() // want `secmem-lint: IsSealed called on the same buffer`
+	})
+}
+
+// differentFieldOK: a different field is a different buffer, so the
+// decrypt-into pattern still has to survive the wider receiver matching.
+func (v *vault) differentFieldOK() {
+	_ = v.buf.WithBytes(func(b []byte) {
+		_ = b
+		_ = v.inner.buf.Len()
+	})
+}
+
+// indexedReceiverNotDecidable: bufs[i] and bufs[j] are written alike and need
+// not be the same buffer, so receivers that are index expressions are left
+// alone rather than guessed at.
+func indexedReceiverNotDecidable(bufs []*secmem.SecureBuffer) {
+	_ = bufs[0].WithBytes(func(b []byte) {
+		_ = b
+		_ = bufs[1].Len()
+	})
+}