cmd/vulnreport,internal/symbols: remove logging from internal/symbols
Return errors instead of logging in internal/symbols functions.
Change-Id: I43efd1e27a97a8f6f42abcc361d647fb9eee8f4c
Reviewed-on: https://go-review.googlesource.com/c/vulndb/+/562196
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Maceo Thompson <maceothompson@google.com>
Commit-Queue: Tatiana Bradley <tatianabradley@google.com>
Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com>
diff --git a/cmd/vulnreport/create.go b/cmd/vulnreport/create.go
index 1634433..39ae5a3 100644
--- a/cmd/vulnreport/create.go
+++ b/cmd/vulnreport/create.go
@@ -230,7 +230,7 @@
}
if *populateSymbols {
- if err := symbols.Populate(r, log.Err); err != nil {
+ if err := symbols.Populate(r); err != nil {
log.Warnf("could not auto-populate symbols: %s", err)
}
}
diff --git a/cmd/vulnreport/fix.go b/cmd/vulnreport/fix.go
index f6d8420..6cdd0c2 100644
--- a/cmd/vulnreport/fix.go
+++ b/cmd/vulnreport/fix.go
@@ -137,7 +137,7 @@
log.Infof("%s: skipping symbol checks for package %s (reason: %q)\n", r.ID, p.Package, p.SkipFix)
continue
}
- syms, err := symbols.Exported(m, p, log.Errf, log.Err)
+ syms, err := symbols.Exported(m, p)
if err != nil {
return fmt.Errorf("package %s: %w", p.Package, err)
}
diff --git a/cmd/vulnreport/symbols.go b/cmd/vulnreport/symbols.go
index bc0aa78..8712bbe 100644
--- a/cmd/vulnreport/symbols.go
+++ b/cmd/vulnreport/symbols.go
@@ -7,7 +7,6 @@
import (
"context"
- "golang.org/x/vulndb/cmd/vulnreport/log"
"golang.org/x/vulndb/internal/report"
"golang.org/x/vulndb/internal/symbols"
)
@@ -31,7 +30,7 @@
return err
}
- if err = symbols.Populate(r, log.Err); err != nil {
+ if err = symbols.Populate(r); err != nil {
return err
}
diff --git a/internal/symbols/exported_functions.go b/internal/symbols/exported_functions.go
index b1ea606..604d466 100644
--- a/internal/symbols/exported_functions.go
+++ b/internal/symbols/exported_functions.go
@@ -6,6 +6,7 @@
import (
"bytes"
+ "errors"
"fmt"
"go/types"
"os"
@@ -23,7 +24,7 @@
// Exported returns a set of vulnerable symbols, in the vuln
// db format, exported by a package p from the module m.
-func Exported(m *report.Module, p *report.Package, errf logf, errln logln) (_ []string, err error) {
+func Exported(m *report.Module, p *report.Package) (_ []string, err error) {
defer derrors.Wrap(&err, "Exported(%q, %q)", m.Module, p.Package)
cleanup, err := changeToTempDir()
@@ -36,9 +37,9 @@
cmd := exec.Command(name, arg...)
out, err := cmd.CombinedOutput()
if err != nil {
- errln(string(out))
+ return fmt.Errorf("%s: %v\nout:\n%s", name, err, string(out))
}
- return err
+ return nil
}
// This procedure was developed through trial and error finding a way
@@ -111,23 +112,8 @@
// Check to see that all symbols actually exist in the package.
// This should perhaps be a lint check, but lint doesn't
// load/typecheck packages at the moment, so do it here for now.
- for _, sym := range p.Symbols {
- if typ, method, ok := strings.Cut(sym, "."); ok {
- n, ok := pkg.Types.Scope().Lookup(typ).(*types.TypeName)
- if !ok {
- errf("package %s: %v: type not found\n", p.Package, typ)
- continue
- }
- m, _, _ := types.LookupFieldOrMethod(n.Type(), true, pkg.Types, method)
- if m == nil {
- errf("package %s: %v: method not found\n", p.Package, sym)
- }
- } else {
- _, ok := pkg.Types.Scope().Lookup(typ).(*types.Func)
- if !ok {
- errf("package %s: %v: func not found\n", p.Package, typ)
- }
- }
+ if err := checkSymbols(pkg, p.Symbols); err != nil {
+ return nil, fmt.Errorf("invalid symbol(s):\n%w", err)
}
newsyms, err := exportedFunctions(pkg, m)
@@ -153,10 +139,28 @@
return newslice, nil
}
-// TODO(tatianabradley): Refactor functions that use these to return
-// info needed to construct logs instead of logging directly.
-type logf func(format string, v ...any)
-type logln func(v ...any)
+func checkSymbols(pkg *packages.Package, symbols []string) error {
+ var errs []error
+ for _, sym := range symbols {
+ if typ, method, ok := strings.Cut(sym, "."); ok {
+ n, ok := pkg.Types.Scope().Lookup(typ).(*types.TypeName)
+ if !ok {
+ errs = append(errs, fmt.Errorf("%v: type not found", typ))
+ continue
+ }
+ m, _, _ := types.LookupFieldOrMethod(n.Type(), true, pkg.Types, method)
+ if m == nil {
+ errs = append(errs, fmt.Errorf("%v: method not found", sym))
+ }
+ } else {
+ _, ok := pkg.Types.Scope().Lookup(typ).(*types.Func)
+ if !ok {
+ errs = append(errs, fmt.Errorf("%v: func not found", typ))
+ }
+ }
+ }
+ return errors.Join(errs...)
+}
// exportedFunctions returns a set of vulnerable functions exported
// by a packages from the module.
diff --git a/internal/symbols/populate.go b/internal/symbols/populate.go
index 840cfdc..fedcf70 100644
--- a/internal/symbols/populate.go
+++ b/internal/symbols/populate.go
@@ -5,6 +5,7 @@
package symbols
import (
+ "errors"
"fmt"
"path/filepath"
"strings"
@@ -15,11 +16,11 @@
// Populate attempts to populate the report with symbols derived
// from the patch link(s) in the report.
-func Populate(r *report.Report, errln logln) error {
- return populate(r, Patched, errln)
+func Populate(r *report.Report) error {
+ return populate(r, Patched)
}
-func populate(r *report.Report, patched func(string, string, string) (map[string][]string, error), errln logln) error {
+func populate(r *report.Report, patched func(string, string, string) (map[string][]string, error)) error {
var defaultFixes []string
for _, ref := range r.References {
@@ -32,7 +33,7 @@
if len(defaultFixes) == 0 {
return fmt.Errorf("no commit fix links found")
}
-
+ var errs []error
for _, mod := range r.Modules {
hasFixLink := mod.FixLink != ""
if hasFixLink {
@@ -44,7 +45,7 @@
fixRepo := strings.TrimSuffix(fixLink, "/commit/"+fixHash)
pkgsToSymbols, err := patched(mod.Module, fixRepo, fixHash)
if err != nil {
- errln(err)
+ errs = append(errs, err)
continue
}
packages := mod.AllPackages()
@@ -69,7 +70,7 @@
}
}
- return nil
+ return errors.Join(errs...)
}
// indexMax takes a slice of nonempty ints and returns the index of the maximum value
diff --git a/internal/symbols/populate_test.go b/internal/symbols/populate_test.go
index 8b35f1d..57e15e0 100644
--- a/internal/symbols/populate_test.go
+++ b/internal/symbols/populate_test.go
@@ -89,8 +89,7 @@
},
} {
t.Run(tc.name, func(t *testing.T) {
- discardLog := func(...any) {}
- if err := populate(tc.input, patchedFake, discardLog); err != nil {
+ if err := populate(tc.input, patchedFake); err != nil {
t.Fatal(err)
}
got := tc.input
