From be300f80bd0ea02f9ef2a4a42bc8346a20ddbe00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Kapka?= Date: Fri, 12 Dec 2025 22:38:09 +0100 Subject: [PATCH] Static analyzer for `httputil.HandleError` calls (#16134) **What type of PR is this?** Tooling **What does the PR do?** Every call to `httputil.HandleError` must be followed by a `return` statement. It's easy to miss this during reviews, so having a static analyzer that enforces this will make our life easier. **Acknowledgements** - [x] I have read [CONTRIBUTING.md](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md). - [x] I have included a uniquely named [changelog fragment file](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md#maintaining-changelogmd). - [x] I have added a description with sufficient context for reviewers to understand this PR. - [x] I have tested that my changes work as expected and I added a testing plan to the PR description (if applicable). --------- Co-authored-by: james-prysm <90280386+james-prysm@users.noreply.github.com> --- BUILD.bazel | 1 + beacon-chain/rpc/eth/light-client/handlers.go | 5 + beacon-chain/rpc/eth/node/handlers.go | 1 + beacon-chain/rpc/eth/validator/handlers.go | 23 ++- changelog/radek_httperror-analyzer.md | 3 + tools/analyzers/httperror/BUILD.bazel | 13 ++ tools/analyzers/httperror/analyzer.go | 163 ++++++++++++++++++ 7 files changed, 197 insertions(+), 12 deletions(-) create mode 100644 changelog/radek_httperror-analyzer.md create mode 100644 tools/analyzers/httperror/BUILD.bazel create mode 100644 tools/analyzers/httperror/analyzer.go diff --git a/BUILD.bazel b/BUILD.bazel index aeb3e553b9..fa57a8afb8 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -193,6 +193,7 @@ nogo( "//tools/analyzers/featureconfig:go_default_library", "//tools/analyzers/gocognit:go_default_library", "//tools/analyzers/ineffassign:go_default_library", + "//tools/analyzers/httperror:go_default_library", "//tools/analyzers/interfacechecker:go_default_library", "//tools/analyzers/logcapitalization:go_default_library", "//tools/analyzers/logruswitherror:go_default_library", diff --git a/beacon-chain/rpc/eth/light-client/handlers.go b/beacon-chain/rpc/eth/light-client/handlers.go index 8a6655f6e3..53bccff78d 100644 --- a/beacon-chain/rpc/eth/light-client/handlers.go +++ b/beacon-chain/rpc/eth/light-client/handlers.go @@ -116,6 +116,7 @@ func (s *Server) GetLightClientUpdatesByRange(w http.ResponseWriter, req *http.R for _, update := range updates { if ctx.Err() != nil { httputil.HandleError(w, "Context error: "+ctx.Err().Error(), http.StatusInternalServerError) + return } updateSlot := update.AttestedHeader().Beacon().Slot @@ -131,12 +132,15 @@ func (s *Server) GetLightClientUpdatesByRange(w http.ResponseWriter, req *http.R chunkLength = ssz.MarshalUint64(chunkLength, uint64(len(updateSSZ)+4)) if _, err := w.Write(chunkLength); err != nil { httputil.HandleError(w, "Could not write chunk length: "+err.Error(), http.StatusInternalServerError) + return } if _, err := w.Write(updateEntry.ForkDigest[:]); err != nil { httputil.HandleError(w, "Could not write fork digest: "+err.Error(), http.StatusInternalServerError) + return } if _, err := w.Write(updateSSZ); err != nil { httputil.HandleError(w, "Could not write update SSZ: "+err.Error(), http.StatusInternalServerError) + return } } } else { @@ -145,6 +149,7 @@ func (s *Server) GetLightClientUpdatesByRange(w http.ResponseWriter, req *http.R for _, update := range updates { if ctx.Err() != nil { httputil.HandleError(w, "Context error: "+ctx.Err().Error(), http.StatusInternalServerError) + return } updateJson, err := structs.LightClientUpdateFromConsensus(update) diff --git a/beacon-chain/rpc/eth/node/handlers.go b/beacon-chain/rpc/eth/node/handlers.go index 665dfa78ac..01d979c0b5 100644 --- a/beacon-chain/rpc/eth/node/handlers.go +++ b/beacon-chain/rpc/eth/node/handlers.go @@ -132,6 +132,7 @@ func (s *Server) GetHealth(w http.ResponseWriter, r *http.Request) { optimistic, err := s.OptimisticModeFetcher.IsOptimistic(ctx) if err != nil { httputil.HandleError(w, "Could not check optimistic status: "+err.Error(), http.StatusInternalServerError) + return } if s.SyncChecker.Synced() && !optimistic { return diff --git a/beacon-chain/rpc/eth/validator/handlers.go b/beacon-chain/rpc/eth/validator/handlers.go index 11f0488a02..e2ea1b523e 100644 --- a/beacon-chain/rpc/eth/validator/handlers.go +++ b/beacon-chain/rpc/eth/validator/handlers.go @@ -1103,7 +1103,8 @@ func (s *Server) GetProposerDuties(w http.ResponseWriter, r *http.Request) { httputil.HandleError(w, "Could not check optimistic status: "+err.Error(), http.StatusInternalServerError) return } - if !sortProposerDuties(w, duties) { + if err = sortProposerDuties(duties); err != nil { + httputil.HandleError(w, "Could not sort proposer duties: "+err.Error(), http.StatusInternalServerError) return } @@ -1447,22 +1448,20 @@ func syncCommitteeDutiesAndVals( return duties, vals, nil } -func sortProposerDuties(w http.ResponseWriter, duties []*structs.ProposerDuty) bool { - ok := true +func sortProposerDuties(duties []*structs.ProposerDuty) error { + var err error sort.Slice(duties, func(i, j int) bool { - si, err := strconv.ParseUint(duties[i].Slot, 10, 64) - if err != nil { - httputil.HandleError(w, "Could not parse slot: "+err.Error(), http.StatusInternalServerError) - ok = false + si, parseErr := strconv.ParseUint(duties[i].Slot, 10, 64) + if parseErr != nil { + err = errors.Wrap(parseErr, "could not parse slot") return false } - sj, err := strconv.ParseUint(duties[j].Slot, 10, 64) - if err != nil { - httputil.HandleError(w, "Could not parse slot: "+err.Error(), http.StatusInternalServerError) - ok = false + sj, parseErr := strconv.ParseUint(duties[j].Slot, 10, 64) + if parseErr != nil { + err = errors.Wrap(parseErr, "could not parse slot") return false } return si < sj }) - return ok + return err } diff --git a/changelog/radek_httperror-analyzer.md b/changelog/radek_httperror-analyzer.md new file mode 100644 index 0000000000..507fe266ef --- /dev/null +++ b/changelog/radek_httperror-analyzer.md @@ -0,0 +1,3 @@ +### Added + +- Static analyzer that ensures each `httputil.HandleError` call is followed by a `return` statement. \ No newline at end of file diff --git a/tools/analyzers/httperror/BUILD.bazel b/tools/analyzers/httperror/BUILD.bazel new file mode 100644 index 0000000000..5df2bb292e --- /dev/null +++ b/tools/analyzers/httperror/BUILD.bazel @@ -0,0 +1,13 @@ +load("@prysm//tools/go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["analyzer.go"], + importpath = "github.com/OffchainLabs/prysm/v7/tools/analyzers/httperror", + visibility = ["//visibility:public"], + deps = [ + "@org_golang_x_tools//go/analysis:go_default_library", + "@org_golang_x_tools//go/analysis/passes/inspect:go_default_library", + "@org_golang_x_tools//go/ast/inspector:go_default_library", + ], +) diff --git a/tools/analyzers/httperror/analyzer.go b/tools/analyzers/httperror/analyzer.go new file mode 100644 index 0000000000..ed8dfa5046 --- /dev/null +++ b/tools/analyzers/httperror/analyzer.go @@ -0,0 +1,163 @@ +package httperror + +import ( + "go/ast" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +var Analyzer = &analysis.Analyzer{ + Name: "httperror", + Doc: "Ensures calls to httputil.HandleError are immediately followed by a return statement.", + Requires: []*analysis.Analyzer{ + inspect.Analyzer, + }, +} + +func Run(pass *analysis.Pass) (any, error) { + ins := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + + ins.Preorder([]ast.Node{(*ast.FuncDecl)(nil)}, func(n ast.Node) { + fn := n.(*ast.FuncDecl) + if fn.Body == nil { + return + } + // top-level: there is no next statement after the function body + checkBlock(pass, fn, fn.Body, nil) + }) + + return nil, nil +} + +func init() { + Analyzer.Run = Run +} + +// checkBlock inspects all statements inside block. +// nextAfter is the statement that comes immediately *after* the statement that contains this block, +// at the enclosing lexical level (or nil if none). +// Example: for `if ... { ... } ; return`, when checking the if's body the nextAfter will be the top-level return stmt. +func checkBlock(pass *analysis.Pass, fn *ast.FuncDecl, block *ast.BlockStmt, nextAfter ast.Stmt) { + stmts := block.List + for i, stmt := range stmts { + // compute the next statement after this statement at the same lexical (ancestor) level: + // if there is a sibling, use it; otherwise propagate the nextAfter we were given. + var nextForThisStmt ast.Stmt + if i+1 < len(stmts) { + nextForThisStmt = stmts[i+1] + } else { + nextForThisStmt = nextAfter + } + + // Recurse into nested blocks BEFORE checking this stmt's own expr, + // but we must pass nextForThisStmt to nested blocks so nested HandleError + // will see the correct "next statement after this statement". + switch s := stmt.(type) { + case *ast.IfStmt: + // pass what's next after the whole if-statement down into its bodies + if s.Init != nil { + // init is a statement (rare), treat it as contained in s; it should use next being the if's body first stmt, + // but for our purposes we don't need to inspect s.Init specially beyond nested calls. + // We'll just check it with nextForThisStmt as well. + checkBlock(pass, fn, &ast.BlockStmt{List: []ast.Stmt{s.Init}}, nextForThisStmt) + } + if s.Body != nil { + checkBlock(pass, fn, s.Body, nextForThisStmt) + } + if s.Else != nil { + switch els := s.Else.(type) { + case *ast.BlockStmt: + checkBlock(pass, fn, els, nextForThisStmt) + case *ast.IfStmt: + // else-if: its body will receive the same nextForThisStmt + checkBlock(pass, fn, els.Body, nextForThisStmt) + } + } + + case *ast.ForStmt: + if s.Init != nil { + checkBlock(pass, fn, &ast.BlockStmt{List: []ast.Stmt{s.Init}}, nextForThisStmt) + } + if s.Body != nil { + checkBlock(pass, fn, s.Body, nextForThisStmt) + } + if s.Post != nil { + checkBlock(pass, fn, &ast.BlockStmt{List: []ast.Stmt{s.Post}}, nextForThisStmt) + } + + case *ast.RangeStmt: + if s.Body != nil { + checkBlock(pass, fn, s.Body, nextForThisStmt) + } + + case *ast.BlockStmt: + // nested block (e.g. anonymous block) — propagate nextForThisStmt + checkBlock(pass, fn, s, nextForThisStmt) + } + + // Now check the current statement itself: is it (or does it contain) a direct call to httputil.HandleError? + // We only consider ExprStmt that are direct CallExpr to httputil.HandleError. + call := findHandleErrorCall(stmt) + if call == nil { + continue + } + + // Determine the actual "next statement after this call" in lexical function order: + // - If there is a sibling in the same block after this stmt, that's next. + // - Otherwise, next is nextForThisStmt (propagated from ancestor). + var nextStmt ast.Stmt + if i+1 < len(stmts) { + nextStmt = stmts[i+1] + } else { + nextStmt = nextAfter + } + + // If there is a next statement and it's a return -> OK + if nextStmt != nil { + if _, ok := nextStmt.(*ast.ReturnStmt); ok { + // immediately followed (in lexical order) by a return at some nesting level -> OK + continue + } + // otherwise it's not a return (even if it's an if/for etc) -> violation + pass.Reportf(stmt.Pos(), "call to httputil.HandleError must be immediately followed by a return statement") + continue + } + + // If nextStmt == nil, this call is lexically the last statement in the function. + // That is allowed only if the function has no result values. + if fn.Type.Results == nil || len(fn.Type.Results.List) == 0 { + // void function: allowed + continue + } + + // Non-void function and it's the last statement → violation + pass.Reportf(stmt.Pos(), "call to httputil.HandleError must be followed by return because function has return values") + } +} + +// findHandleErrorCall returns the call expression if stmt is a direct call to httputil.HandleError(...), +// otherwise nil. We only match direct ExprStmt -> CallExpr -> SelectorExpr where selector is httputil.HandleError. +func findHandleErrorCall(stmt ast.Stmt) *ast.CallExpr { + es, ok := stmt.(*ast.ExprStmt) + if !ok { + return nil + } + call, ok := es.X.(*ast.CallExpr) + if !ok { + return nil + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return nil + } + pkgIdent, ok := sel.X.(*ast.Ident) + if !ok { + return nil + } + if pkgIdent.Name == "httputil" && sel.Sel.Name == "HandleError" { + return call + } + return nil +}