diff --git a/pkg/ociinstaller/fdw.go b/pkg/ociinstaller/fdw.go index 37d501fef..d9e469714 100644 --- a/pkg/ociinstaller/fdw.go +++ b/pkg/ociinstaller/fdw.go @@ -66,13 +66,36 @@ func installFdwFiles(image *ociinstaller.OciImage[*fdwImage, *FdwImageConfig], t // NOTE: for Mac M1 machines, if the fdw binary is updated in place without deleting the existing file, // the updated fdw may crash on execution - for an undetermined reason - // to avoid this, first remove the existing .so file - os.Remove(fdwBinFileDestPath) - // now unzip the fdw file + // To avoid this AND prevent leaving the system without a binary if extraction fails, + // we use a two-phase approach: + // 1. Extract to the target directory first + // 2. Verify extraction succeeded + // 3. Only delete the old binary after verifying the new one was successfully extracted + // 4. Atomically move the new binary into place + + // Extract to target directory first if _, err := ociinstaller.Ungzip(fdwBinFileSourcePath, fdwBinDir); err != nil { return fmt.Errorf("could not unzip %s to %s: %s", fdwBinFileSourcePath, fdwBinDir, err.Error()) } + // Verify extraction succeeded by checking if the extracted file exists + extractedPath := filepath.Join(fdwBinDir, constants.FdwBinaryFileName) + if _, err := os.Stat(extractedPath); err != nil { + return fmt.Errorf("ungzip succeeded but binary not found at %s: %s", extractedPath, err.Error()) + } + + // Move extracted file to temp name to prepare for atomic swap + tempBinaryPath := filepath.Join(fdwBinDir, constants.FdwBinaryFileName+".tmp") + if err := os.Rename(extractedPath, tempBinaryPath); err != nil { + return fmt.Errorf("could not rename extracted binary to temp location: %s", err.Error()) + } + + // NOW it's safe to remove the old binary and move new one into place + os.Remove(fdwBinFileDestPath) + if err := os.Rename(tempBinaryPath, fdwBinFileDestPath); err != nil { + return fmt.Errorf("could not install binary: %s", err.Error()) + } + fdwControlDir := filepaths.GetFDWSQLAndControlDir() controlFileName := image.Data.ControlFile controlFileSourcePath := filepath.Join(tempdir, controlFileName) diff --git a/pkg/ociinstaller/fdw_test.go b/pkg/ociinstaller/fdw_test.go index b2f958d16..c07c22c9f 100644 --- a/pkg/ociinstaller/fdw_test.go +++ b/pkg/ociinstaller/fdw_test.go @@ -122,3 +122,62 @@ func TestMediaTypeProvider_PlatformDetection(t *testing.T) { }) } } + +// TestInstallFdwFiles_CorruptGzipFile_BugDocumentation documents bug #4753 +// This test documents the critical bug where the existing FDW binary was deleted +// before verifying that the new binary could be successfully extracted. +// +// Bug Scenario (BEFORE FIX): +// 1. User has working FDW v1.0 installed +// 2. Upgrade to v2.0 begins +// 3. os.Remove() deletes the v1.0 binary (line 70 in fdw.go) +// 4. Ungzip() attempts to extract v2.0 binary (line 72) +// 5. If ungzip fails (corrupt download, disk full, etc.): +// - Old v1.0 binary is GONE (deleted in step 3) +// - New v2.0 binary FAILED to install (step 4) +// - System is now BROKEN with no FDW at all +// +// This test simulates the old buggy behavior for documentation purposes. +// It is skipped because it will always fail (it simulates the bug itself). +// The fix ensures this scenario can never happen in the actual code. +func TestInstallFdwFiles_CorruptGzipFile_BugDocumentation(t *testing.T) { + t.Skip("Documentation test - simulates the bug that existed before fix #4753") + + // Setup: Create temp directories to simulate FDW installation directories + tempInstallDir := t.TempDir() + tempSourceDir := t.TempDir() + + // Create a valid "existing" FDW binary (v1.0) + existingBinaryPath := filepath.Join(tempInstallDir, "steampipe-postgres-fdw.so") + existingBinaryContent := []byte("existing FDW v1.0 binary") + if err := os.WriteFile(existingBinaryPath, existingBinaryContent, 0755); err != nil { + t.Fatalf("Failed to create existing FDW binary: %v", err) + } + + // Create a CORRUPT gzip file (not a valid gzip) that will fail to ungzip + corruptGzipPath := filepath.Join(tempSourceDir, "steampipe-postgres-fdw.so.gz") + corruptGzipContent := []byte("this is not a valid gzip file, ungzip will fail") + if err := os.WriteFile(corruptGzipPath, corruptGzipContent, 0644); err != nil { + t.Fatalf("Failed to create corrupt gzip file: %v", err) + } + + // Simulate the OLD BUGGY behavior from installFdwFiles() (before fix): + // 1. Remove the old binary first + // 2. Then try to ungzip (which will fail with our corrupt file) + os.Remove(existingBinaryPath) + _, ungzipErr := ociinstaller.Ungzip(corruptGzipPath, tempInstallDir) + + // Verify ungzip failed (confirms test setup) + if ungzipErr == nil { + t.Fatal("Expected ungzip to fail with corrupt file, but it succeeded") + } + + // CRITICAL ASSERTION: After a failed ungzip, the old binary should still exist + // But with the buggy code, it's gone! + _, statErr := os.Stat(existingBinaryPath) + if os.IsNotExist(statErr) { + // This demonstrates the bug: The old binary was deleted BEFORE verifying + // that the new binary could be successfully extracted. + t.Errorf("CRITICAL BUG: Old FDW binary was deleted before new binary extraction succeeded. System left in broken state with no FDW binary.") + } +}