avoid a filesystem race with build cache entries

After the last commit, I started seeing seemingly random test failures:

    --- FAIL: TestScripts/cgo (1.17s)
        testscript.go:397:
            > env GOPRIVATE=test/main
            > garble build
            [stderr]
            # runtime/internal/math
            EOF
            # internal/bytealg
            EOF
            exit status 2
            [exit status 1]
            FAIL: testdata/scripts/cgo.txt:3: unexpected command failure

    --- FAIL: TestScripts/reflect (8.63s)
        testscript.go:397:
            > env GOPRIVATE=test/main
            > garble build
            [stderr]
            # math
            EOF
            exit status 2
            [exit status 1]
            FAIL: testdata/scripts/reflect.txt:3: unexpected command failure

    --- FAIL: TestScripts/linkname (8.72s)
        testscript.go:397:
            > env GOPRIVATE=test/main
            > garble build
            [stderr]
            # math
            EOF
            exit status 2
            [exit status 1]
            FAIL: testdata/scripts/linkname.txt:3: unexpected command failure

After some investigation,
it turned out that concurrent "garble build" processes
were writing to the same build cache paths at the same time.
This is effectively a filesystem race,
and encoding/gob could error when reading partly written files.

To fix this problem,
use a cache path that changes according to the obfuscated build.
See garbleExportFile.

Note that the data we store in that file does not vary with obfuscation.
We could fix the filesystem race by adding locking around the old path.
However, we'll want to cache data that does vary with garble flags,
such as the -debugdir source code.
pull/400/head
Daniel Martí 4 years ago committed by lu4p
parent 8d162dcd04
commit 08ec70e9a9

@ -658,11 +658,11 @@ func transformCompile(args []string) ([]string, error) {
return nil, err return nil, err
} }
if err = loadKnownReflectAPIs(curPkg); err != nil { if err := loadKnownReflectAPIs(); err != nil {
return nil, err return nil, err
} }
tf.findReflectFunctions(files) tf.findReflectFunctions(files)
if err := saveKnownReflectAPIs(curPkg); err != nil { if err := saveKnownReflectAPIs(); err != nil {
return nil, err return nil, err
} }
@ -1036,16 +1036,37 @@ var knownReflectAPIs = map[funcFullName][]reflectParameterPosition{
"reflect.ValueOf": {0}, "reflect.ValueOf": {0},
} }
func loadKnownReflectAPIs(currPkg *listedPackage) error { // garbleExportFile returns an absolute path to a build cache entry
for path := range importCfgEntries { // which belongs to garble and corresponds to the given Go package.
//
// Unlike pkg.Export, it is only read and written by garble itself.
// Also unlike pkg.Export, it includes GarbleActionID,
// so its path will change if the obfuscated build changes.
//
// The purpose of such a file is to store garble-specific information
// in the build cache, to be reused at a later time.
// The file should have the same lifetime as pkg.Export,
// as it lives under the same cache directory that gets trimmed automatically.
func garbleExportFile(pkg *listedPackage) string {
trimmed := strings.TrimSuffix(pkg.Export, "-d")
if trimmed == pkg.Export {
panic(fmt.Sprintf("unexpected export path of %s: %q", pkg.ImportPath, pkg.Export))
}
return trimmed + "-garble-" + hashToString(pkg.GarbleActionID) + "-d"
}
func loadKnownReflectAPIs() error {
for _, path := range curPkg.Deps {
pkg, err := listPackage(path) pkg, err := listPackage(path)
if err != nil { if err != nil {
return err return err
} }
if pkg.Export == "" {
continue // nothing to load
}
// this function literal is used for the deferred close // this function literal is used for the deferred close
err = func() error { err = func() error {
filename := strings.TrimSuffix(pkg.Export, "-d") + "-garble-d" filename := garbleExportFile(pkg)
f, err := os.Open(filename) f, err := os.Open(filename)
if err != nil { if err != nil {
return err return err
@ -1053,7 +1074,10 @@ func loadKnownReflectAPIs(currPkg *listedPackage) error {
defer f.Close() defer f.Close()
// Decode appends new entries to the existing map // Decode appends new entries to the existing map
return gob.NewDecoder(f).Decode(&knownReflectAPIs) if err := gob.NewDecoder(f).Decode(&knownReflectAPIs); err != nil {
return fmt.Errorf("gob decode: %w", err)
}
return nil
}() }()
if err != nil { if err != nil {
return err return err
@ -1063,15 +1087,18 @@ func loadKnownReflectAPIs(currPkg *listedPackage) error {
return nil return nil
} }
func saveKnownReflectAPIs(currPkg *listedPackage) error { func saveKnownReflectAPIs() error {
filename := strings.TrimSuffix(currPkg.Export, "-d") + "-garble-d" filename := garbleExportFile(curPkg)
f, err := os.Create(filename) f, err := os.Create(filename)
if err != nil { if err != nil {
return err return err
} }
defer f.Close() defer f.Close()
return gob.NewEncoder(f).Encode(knownReflectAPIs) if err := gob.NewEncoder(f).Encode(knownReflectAPIs); err != nil {
return fmt.Errorf("gob encode: %w", err)
}
return f.Close()
} }
func (tf *transformer) findReflectFunctions(files []*ast.File) { func (tf *transformer) findReflectFunctions(files []*ast.File) {

Loading…
Cancel
Save