Skip to content

Fix Windows permission error with self remove #52316

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.

Sign up for GitHub

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

Merged
vapopov merged 3 commits into from
Feb 25, 2025
Merged

Fix Windows permission error with self remove #52316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bef7c9b
Fix windows permission error with self replace
vapopov Feb 19, 2025
c005fbf
Aggregate errors
vapopov Feb 24, 2025
506ba07
Update comments
vapopov Feb 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions integration/autoupdate/tools/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ var (

func TestMain(m *testing.M) {
modules.SetInsecureTestMode(true)
modules.SetModules(&modules.TestModules{TestBuildType: modules.BuildCommunity})
ctx := context.Background()
tmp, err := os.MkdirTemp(os.TempDir(), testBinaryName)
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions integration/autoupdate/tools/updater/modules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,12 @@ func (p *TestModules) BuildType() string {
return "CLI"
}

// IsEnterpriseBuild returns false for [TestModules].
// IsEnterpriseBuild returns true if `UPDATER_TEST_BUILD` env is set `ent` for [TestModules].
func (p *TestModules) IsEnterpriseBuild() bool {
return os.Getenv(TestBuild) == modules.BuildEnterprise
}

// IsOSSBuild returns false for [TestModules].
// IsOSSBuild returns true if `UPDATER_TEST_BUILD` env is set `oss` for [TestModules].
func (p *TestModules) IsOSSBuild() bool {
return os.Getenv(TestBuild) == modules.BuildOSS
}
Expand Down
2 changes: 2 additions & 0 deletions integration/autoupdate/tools/updater_tsh_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
//go:build !windows

/*
* Teleport
* Copyright (C) 2024 Gravitational, Inc.
Expand Down
6 changes: 5 additions & 1 deletion lib/utils/fs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,15 +422,19 @@ func RecursiveChown(dir string, uid, gid int) error {
return nil
}

func CopyFile(src, dest string, perm os.FileMode) error {
func CopyFile(src, dest string, perm os.FileMode) (err error) {
srcFile, err := os.Open(src)
if err != nil {
return trace.ConvertSystemError(err)
}
defer srcFile.Close()
destFile, err := os.OpenFile(dest, os.O_RDWR|os.O_CREATE|os.O_TRUNC, perm)
if err != nil {
return trace.ConvertSystemError(err)
}
defer func() {
err = trace.NewAggregate(err, trace.Wrap(destFile.Close()))
Copy link
Copy Markdown
Member

@ravicious ravicious Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we only propagate the error from destFile.Close() and not srcFile.Close()? I don't know what's the correct behavior here, it's just something that caught my eye.

Copy link
Copy Markdown
Contributor Author

@vapopov vapopov Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

destFile.Close() errors can happen if there’s a problem flushing the final buffered data to disk, like running out of space, this error is crucial because it directly impacts whether the data was successfully written and saved. srcFile is opened for reading and without error propagation because failure there doesn’t affect data integrity. Should I add the logging for srcFile what do you think?

}()
_, err = destFile.ReadFrom(srcFile)
if err != nil {
return trace.ConvertSystemError(err)
Expand Down
10 changes: 4 additions & 6 deletions lib/utils/packaging/unarchive.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,10 @@ func replaceZip(toolsDir string, archivePath string, extractDir string, execName
return trace.Wrap(err)
}
appPath := filepath.Join(toolsDir, baseName)
if err := os.Remove(appPath); err != nil && !os.IsNotExist(err) {
return trace.Wrap(err)
}
// For the Windows build we have to copy binary to be able
// to do this without administrative access as it required
// for symlinks.
// For the Windows build, we need to copy the binary to perform updates without requiring
// administrative access, which would otherwise be needed for creating symlinks.
// Since symlinks are not used on the Windows platform, there's no need to remove appPath
// before copying the new binary — it will simply be replaced.
if err := utils.CopyFile(dest, appPath, 0o755); err != nil {
return trace.Wrap(err)
}
Expand Down