diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 13db00c..799f014 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -204,6 +204,9 @@ Focused check: `go test ./internal/engine -run 'Callback|Resolution|Validation|V Finding: **B02**. Owners: `internal/engine/import_acquisition.go`, `filesystem_access.go`, import tests. +Status: **complete** (B02). Prior stage commits: `2736ddf`, `deb9aa5`, +`ae3408a`, `ea27f17`, `08a8c25`. Validation passed: `GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off go test ./internal/engine -run 'Import|SourceVersion|Filesystem' -count=1` and `GOTOOLCHAIN=local GOPROXY=off GOTELEMETRY=off go test ./internal/engine -count=1`. + Install deferred fallback closure at the entry of `acquireImportedWithAccess`, before operations that may invoke application code. Use a local closed flag or one local close-once function so existing explicit close sites preserve their diff --git a/internal/engine/import_acquisition.go b/internal/engine/import_acquisition.go index eaab38b..8b4c9c5 100644 --- a/internal/engine/import_acquisition.go +++ b/internal/engine/import_acquisition.go @@ -61,6 +61,16 @@ func acquireImportedInput(request importAcquisitionRequest, policy *acquisitionP } func acquireImportedWithAccess(request importAcquisitionRequest, policy *acquisitionPolicy, loadState *acquisitionState, tier *tierAcquisitionState, access *filesystemAccess, resolved resolvedFilesystemContext) (*composedDocument, []SourceInfo, []Diagnostic) { + closed := false + closeAccess := func() error { + if closed { + return nil + } + closed = true + return access.close() + } + defer func() { _ = closeAccess() }() + main := request.main destination := request.input.input.destination limits := policy.importLimits @@ -73,13 +83,13 @@ func acquireImportedWithAccess(request importAcquisitionRequest, policy *acquisi } visited := tier.visited if !access.contains(basePath) || (mainPath != "" && !access.contains(mainPath)) { - _ = access.close() + _ = closeAccess() return nil, sourceInfos(main.source), []Diagnostic{importDiagnostic(CodeImportOutsideRoot, "selected source and import base must stay inside the configuration root", nil)} } for _, directory := range []string{access.rootPath, basePath} { info, err := access.stat(directory) if err != nil || !info.IsDir() { - _ = access.close() + _ = closeAccess() return nil, sourceInfos(main.source), []Diagnostic{importDiagnostic(CodeImportInvalid, "configuration root and import base must be directories", nil)} } } @@ -95,7 +105,7 @@ func acquireImportedWithAccess(request importAcquisitionRequest, policy *acquisi mainResource = access.syntheticIdentity() } if previous, repeated := visited[mainResource]; repeated { - _ = access.close() + _ = closeAccess() return nil, sourceInfos(main.source), []Diagnostic{importRepeatedDiagnostic(Location{SourceID: main.source.ID}, previous, importRoute{})} } accounting.sources++ @@ -125,7 +135,7 @@ func acquireImportedWithAccess(request importAcquisitionRequest, policy *acquisi } if diagnostic := state.checkInitialLimits(); diagnostic != nil { - _ = access.close() + _ = closeAccess() return nil, cloneSourceInfos(state.sources), []Diagnostic{*diagnostic} } @@ -181,7 +191,7 @@ func acquireImportedWithAccess(request importAcquisitionRequest, policy *acquisi scheduleAcquisitionScopes(&state.scope, indexImportDependencies(state.importGraph)) } - closeErr := access.close() + closeErr := closeAccess() if len(diagnostics) != 0 { if mainIsItem { diagnostics = prependCompositionDiagnosticPaths(diagnostics, destination.Index(0)) diff --git a/internal/engine/import_root_panic_test.go b/internal/engine/import_root_panic_test.go new file mode 100644 index 0000000..cb4327f --- /dev/null +++ b/internal/engine/import_root_panic_test.go @@ -0,0 +1,108 @@ +package engine + +import ( + "errors" + "os" + "path/filepath" + "testing" + "testing/fstest" +) + +func TestImportedAcquisitionClosesOwnedHostRootOnEveryExit(t *testing.T) { + tests := []struct { + name string + main string + child string + version bool + wantError bool + wantPanic bool + }{ + {name: "success", main: "configkit: {imports: [{path: child.yml}]}\n", child: "value: child\n"}, + {name: "error", main: "configkit: {imports: [{path: missing.yml, required: true}]}\n", wantError: true}, + {name: "version panic", main: "configkit: {imports: [{path: child.yml}]}\n", child: "version: v1\n", version: true, wantPanic: true}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + rootPath := t.TempDir() + mainPath := filepath.Join(rootPath, "main.yml") + if err := os.WriteFile(mainPath, []byte(test.main), 0o600); err != nil { + t.Fatal(err) + } + if test.child != "" { + if err := os.WriteFile(filepath.Join(rootPath, "child.yml"), []byte(test.child), 0o600); err != nil { + t.Fatal(err) + } + } + source := File(mainPath) + main, diagnostics := parseSource(source, "source-1", defaultParseLimits()) + if len(diagnostics) != 0 { + t.Fatalf("main parse diagnostics = %#v", diagnostics) + } + access, err := openHostAccess(rootPath) + if err != nil { + t.Fatal(err) + } + ownedRoot := access.root + sentinel := errors.New("imported version checker panic sentinel") + var rule *VersionRule + var versionState *sourceVersionState + if test.version { + rule = &VersionRule{Path: Keys("version"), Check: func(Version, *Validation) error { panic(sentinel) }} + versionState = &sourceVersionState{localRule: rule, effectiveRule: rule, mode: sourceVersionModeRequired} + } + request := testImportRequest(source, nil, main, Path{}, rule, nil) + policy := &acquisitionPolicy{importLimits: defaultImportLimits()} + state := &acquisitionState{accounting: &importAccounting{}, versionState: versionState} + tier := &tierAcquisitionState{} + resolved := resolvedFilesystemContext{rootPath: rootPath, basePath: rootPath, mainPath: mainPath} + var resultDiagnostics []Diagnostic + var recovered any + func() { + defer func() { recovered = recover() }() + _, _, resultDiagnostics = acquireImportedWithAccess(request, policy, state, tier, access, resolved) + }() + if test.wantPanic { + if recovered != sentinel { + t.Fatalf("recovered panic = %#v, want original sentinel", recovered) + } + } else if recovered != nil { + t.Fatalf("unexpected panic = %#v", recovered) + } + if test.wantError != (len(resultDiagnostics) != 0) { + t.Fatalf("diagnostics = %#v, want error=%v", resultDiagnostics, test.wantError) + } + if _, err := ownedRoot.Stat("."); err == nil { + t.Fatal("owned host root remained usable after acquisition returned") + } + }) + } +} + +func TestImportedAcquisitionClosesChildFileBeforeVersionPanic(t *testing.T) { + filesystem := &observedFilesystem{files: fstest.MapFS{ + "main.yml": &fstest.MapFile{Data: []byte("configkit: {imports: [{path: child.yml}]}\n")}, + "child.yml": &fstest.MapFile{Data: []byte("version: v1\n")}, + }} + namespace := newNativeNamespace("bundle", filesystem) + source := filesystemFile(namespace, "main.yml") + main, diagnostics := parseSource(source, "source-1", defaultParseLimits()) + if len(diagnostics) != 0 { + t.Fatalf("main parse diagnostics = %#v", diagnostics) + } + sentinel := errors.New("imported version checker panic sentinel") + rule := &VersionRule{Path: Keys("version"), Check: func(Version, *Validation) error { panic(sentinel) }} + versionState := &sourceVersionState{localRule: rule, effectiveRule: rule, mode: sourceVersionModeRequired} + request := testImportRequest(source, nil, main, Path{}, rule, nil) + access := newNativeAccess(filesystem, ".", namespace.token) + var recovered any + func() { + defer func() { recovered = recover() }() + _, _, _ = acquireImportedWithAccess(request, &acquisitionPolicy{importLimits: defaultImportLimits()}, + &acquisitionState{accounting: &importAccounting{}, versionState: versionState}, &tierAcquisitionState{}, access, + resolvedFilesystemContext{native: namespace, rootPath: ".", basePath: ".", mainPath: source.path}) + }() + if recovered != sentinel { + t.Fatalf("recovered panic = %#v, want original sentinel", recovered) + } + filesystem.assertClosed(t) +}