Skip to content
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

sysfs: reopening file doesn't update fd #2319

Merged
merged 8 commits into from
Sep 23, 2024
Merged

sysfs: reopening file doesn't update fd #2319

merged 8 commits into from
Sep 23, 2024

Conversation

ncruces
Copy link
Collaborator

@ncruces ncruces commented Sep 23, 2024

Fixes #2318.

Reopening the file was erroneously not updating the fd field.
Also, reopening is risky, because the file have been deleted/renamed (yes, even on Windows, because we're using FILE_SHARE_DELETE).

In theory, on POSIX, we'd never need to reopen (hence why WASI provides this, it tries to offer much of what POSIX allows, neglecting that lots of it is hard to emulate). But... reopen is only is only really used in two places: seeking directories (to rewind them), and setting the file's append flag.

For directory rewind, it's already just a fallback:

func (f *osFile) Seek(offset int64, whence int) (newOffset int64, errno experimentalsys.Errno) {
if newOffset, errno = seek(f.file, offset, whence); errno != 0 {
// Defer validation overhead until we've already had an error.
errno = fileError(f, f.closed, errno)
// If the error was trying to rewind a directory, re-open it. Notably,
// seeking to zero on a directory doesn't work on Windows with Go 1.19.
if errno == experimentalsys.EISDIR && offset == 0 && whence == io.SeekStart {
errno = 0
f.reopenDir = true
}
}
return
}

If seeking works, it's used. So, no point in trying to "fix" that one.

For setting the append flag, reopen is always used, for good reason:

// SetAppend implements the same method as documented on sys.File
func (f *osFile) SetAppend(enable bool) (errno experimentalsys.Errno) {
if enable {
f.flag |= experimentalsys.O_APPEND
} else {
f.flag &= ^experimentalsys.O_APPEND
}
// Clear any create or trunc flag, as we are re-opening, not re-creating.
f.flag &= ^(experimentalsys.O_CREAT | experimentalsys.O_TRUNC)
// appendMode (bool) cannot be changed later, so we have to re-open the
// file. https://github.com/golang/go/blob/go1.20/src/os/file_unix.go#L60
return fileError(f, f.closed, f.reopen())
}

That link is still valid in 1.23. Go's os.File caches this, most importantly, to return a specific error on WriteAt. There's a good reason for this. The Open Group specifies that pwrite should work with, and ignore, O_APPEND, but Linux does the opposite and appends, which is a bug.

So (IMO) we do need to reopen for SetAppend, and the best we can do, is check if the new file is still the same file, and error out otherwise.

That's what this PR does for osFile:

  1. fix the fd not being updated issue
  2. check that reopen gets the same file

For fsFile, this is only used to rewind directories, so:

  1. own that
  2. check that the reopened file is still a directory

Signed-off-by: Nuno Cruces <[email protected]>
Signed-off-by: Nuno Cruces <[email protected]>
Signed-off-by: Nuno Cruces <[email protected]>
Signed-off-by: Nuno Cruces <[email protected]>
Signed-off-by: Nuno Cruces <[email protected]>
Signed-off-by: Nuno Cruces <[email protected]>
Signed-off-by: Nuno Cruces <[email protected]>
Signed-off-by: Nuno Cruces <[email protected]>
@@ -418,13 +418,6 @@ func seek(s io.Seeker, offset int64, whence int) (int64, experimentalsys.Errno)
return newOffset, experimentalsys.UnwrapOSError(err)
}

// reopenFile allows re-opening a file for reasons such as applying flags or
// directory iteration.
type reopenFile func() experimentalsys.Errno
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This wasn't being used anywhere. The compile time check is not very useful, tbh.

var err error
f.file, err = f.fs.Open(f.name)
return experimentalsys.UnwrapOSError(err)
func (f *fsFile) rewindDir() experimentalsys.Errno {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fsFile only really supports reopening dirs, to rewind them.
Own that, check that the reopened file is still a dir.

@@ -83,21 +83,12 @@ func (f *osFile) SetAppend(enable bool) (errno experimentalsys.Errno) {
f.flag &= ^experimentalsys.O_APPEND
}

// Clear any create or trunc flag, as we are re-opening, not re-creating.
f.flag &= ^(experimentalsys.O_CREAT | experimentalsys.O_TRUNC)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't needlessly update the file/flags.

@mathetake
Copy link
Member

awesome detective work!

@mathetake mathetake merged commit dc058c0 into main Sep 23, 2024
44 checks passed
@mathetake mathetake deleted the reopen branch September 23, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fd_fdstat_set_flags can silently change a fd's number
2 participants