-
Notifications
You must be signed in to change notification settings - Fork 105
Fix windows redaction clean #1876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3342ef8
0782305
118d1f0
60bf2ed
35aaac0
871fd05
c139040
1dfbdd9
233a875
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,9 @@ import ( | |
"os" | ||
"path" | ||
"path/filepath" | ||
"runtime" | ||
"strings" | ||
"time" | ||
|
||
"github.com/pkg/errors" | ||
"k8s.io/klog/v2" | ||
|
@@ -189,7 +191,20 @@ func (r CollectorResult) ReplaceResult(bundlePath string, relativePath string, r | |
} | ||
|
||
// Create a temporary file in the same directory as the target file to prevent cross-device issues | ||
tmpFile, err := os.CreateTemp("", "replace-") | ||
var tmpFile *os.File | ||
var err error | ||
|
||
if runtime.GOOS == "windows" { | ||
// Windows-only: Use destination directory to avoid antivirus issues | ||
destDir := filepath.Dir(filepath.Join(bundlePath, relativePath)) | ||
if err := os.MkdirAll(destDir, 0755); err != nil { | ||
return errors.Wrap(err, "failed to create destination directory") | ||
} | ||
tmpFile, err = os.CreateTemp(destDir, "replace-") | ||
} else { | ||
// Linux/macOS: EXACT original behavior - system temp | ||
tmpFile, err = os.CreateTemp("", "replace-") | ||
} | ||
if err != nil { | ||
return errors.Wrap(err, "failed to create temp file") | ||
} | ||
|
@@ -204,9 +219,21 @@ func (r CollectorResult) ReplaceResult(bundlePath string, relativePath string, r | |
tmpFile.Close() | ||
|
||
// This rename should always be in /tmp, so no cross-partition copying will happen | ||
err = os.Rename(tmpFile.Name(), filepath.Join(bundlePath, relativePath)) | ||
if runtime.GOOS == "windows" { | ||
// Windows-specific handling with delete-first approach | ||
finalPath := filepath.Join(bundlePath, relativePath) | ||
|
||
// Delete target file first (Windows requirement to release locks) | ||
os.Remove(finalPath) | ||
|
||
// Use copy+delete instead of rename (more reliable on Windows) | ||
err = copyFileWindows(tmpFile.Name(), finalPath) | ||
} else { | ||
// Linux/macOS: EXACT original behavior - DO NOT CHANGE | ||
err = os.Rename(tmpFile.Name(), filepath.Join(bundlePath, relativePath)) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Windows File Replacement Fails AtomicallyThe Windows-specific file replacement logic is not atomic. It deletes the target file before copying the temporary file, which can lead to permanent data loss if the copy operation fails. The original There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like bugbot is right about this to me, the file is removed and then its copied? Shouldnt it be copied before its deleted? |
||
if err != nil { | ||
return errors.Wrap(err, "failed to rename tmp file") | ||
return errors.Wrap(err, "failed to replace file") | ||
} | ||
|
||
return nil | ||
|
@@ -417,3 +444,62 @@ func TarSupportBundleDir(bundlePath string, input CollectorResult, outputFilenam | |
// Is this used anywhere external anyway? | ||
return input.ArchiveBundle(bundlePath, outputFilename) | ||
} | ||
|
||
// copyFileWindows performs safer file replacement for Windows | ||
// It writes to a temporary file first to avoid data loss if copy fails | ||
func copyFileWindows(src, dst string) error { | ||
// Step 1: Write to temp file with unique name (not touching original) | ||
tmpDst := dst + ".tmp" | ||
|
||
srcFile, err := os.Open(src) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
tmpFile, err := os.Create(tmpDst) | ||
if err != nil { | ||
srcFile.Close() | ||
return err | ||
} | ||
|
||
// Copy data to temp file | ||
_, err = io.Copy(tmpFile, srcFile) | ||
|
||
// Close both files explicitly | ||
srcFile.Close() | ||
tmpFile.Close() | ||
|
||
if err != nil { | ||
// Copy failed - original file is still intact! | ||
os.Remove(tmpDst) | ||
return err | ||
} | ||
|
||
// Step 2: Replace original with temp (with retry for file locking) | ||
// Try up to 5 times with increasing delays for antivirus/locking issues | ||
maxRetries := 5 | ||
for attempt := 0; attempt < maxRetries; attempt++ { | ||
// Delete original to release locks | ||
os.Remove(dst) | ||
|
||
// Rename temp to final | ||
err = os.Rename(tmpDst, dst) | ||
if err == nil { | ||
// Success! Clean up source | ||
os.Remove(src) | ||
return nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Windows File Copy VulnerabilityThe |
||
} | ||
|
||
// If not last attempt, wait with exponential backoff | ||
if attempt < maxRetries-1 { | ||
delay := time.Duration(10*(attempt+1)) * time.Millisecond | ||
time.Sleep(delay) | ||
} | ||
} | ||
|
||
// All retries failed - clean up dst temp file but keep src | ||
// We intentionally leave src (the source temp file) to prevent data loss | ||
// It will be cleaned up when the parent temp directory is removed | ||
os.Remove(tmpDst) | ||
return errors.Wrap(err, "failed to replace file after retries") | ||
} | ||
bennyyang11 marked this conversation as resolved.
Show resolved
Hide resolved
bennyyang11 marked this conversation as resolved.
Show resolved
Hide resolved
|
Uh oh!
There was an error while loading. Please reload this page.