From 2dfadcbaf4605da7fa42d323789e029bc154f024 Mon Sep 17 00:00:00 2001 From: Nuno Cruces Date: Sun, 22 Sep 2024 11:10:58 +0100 Subject: [PATCH 1/8] Issue #2318. Signed-off-by: Nuno Cruces --- internal/sysfs/file_test.go | 21 +++++++++++++++++++++ internal/sysfs/osfile.go | 1 + 2 files changed, 22 insertions(+) diff --git a/internal/sysfs/file_test.go b/internal/sysfs/file_test.go index 425cf34205..8e69921fc2 100644 --- a/internal/sysfs/file_test.go +++ b/internal/sysfs/file_test.go @@ -116,6 +116,26 @@ func TestWriteFdNonblock(t *testing.T) { t.Fatal("writeFd should return EAGAIN at some point") } +func TestFileReopenFileUpdatesFD(t *testing.T) { + tmpDir := t.TempDir() + path := path.Join(tmpDir, "file") + + // Open the file twice. Closing the first file, frees its FD. + // Then reopening the second file will take over the first FD. + // If reopening the file doesn't update the FD, they won't match. + f0 := requireOpenFile(t, path, experimentalsys.O_RDWR|experimentalsys.O_CREAT, 0o600) + f1 := requireOpenFile(t, path, experimentalsys.O_RDWR|experimentalsys.O_CREAT, 0o600) + defer f1.Close() + f0.Close() + + of, ok := f1.(*osFile) + require.True(t, ok) + + errno := of.reopen() + require.EqualErrno(t, 0, errno) + require.Equal(t, of.file.Fd(), of.fd) +} + func TestFileSetAppend(t *testing.T) { tmpDir := t.TempDir() @@ -124,6 +144,7 @@ func TestFileSetAppend(t *testing.T) { // Open without APPEND. f, errno := OpenOSFile(fPath, experimentalsys.O_RDWR, 0o600) + defer f.Close() require.EqualErrno(t, 0, errno) require.False(t, f.IsAppend()) diff --git a/internal/sysfs/osfile.go b/internal/sysfs/osfile.go index 490f0fa681..169d11bcde 100644 --- a/internal/sysfs/osfile.go +++ b/internal/sysfs/osfile.go @@ -118,6 +118,7 @@ func (f *osFile) reopen() (errno experimentalsys.Errno) { _ = f.close() f.file, errno = OpenFile(f.path, f.flag, f.perm) + f.fd = f.file.Fd() if errno != 0 { return errno } From 9624e9d4e886917aba6164b095b12c06dd883abb Mon Sep 17 00:00:00 2001 From: Nuno Cruces Date: Mon, 23 Sep 2024 11:46:03 +0100 Subject: [PATCH 2/8] Docs. Signed-off-by: Nuno Cruces --- sys/stat.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sys/stat.go b/sys/stat.go index bb7b9e5d33..3288855453 100644 --- a/sys/stat.go +++ b/sys/stat.go @@ -29,9 +29,7 @@ type EpochNanos = int64 // # Notes // // - This is used for WebAssembly ABI emulating the POSIX `stat` system call. -// Fields included are required for WebAssembly ABI including wasip1 -// (a.k.a. wasix) and wasi-filesystem (a.k.a. wasip2). See -// https://pubs.opengroup.org/onlinepubs/9699919799/functions/stat.html +// See https://pubs.opengroup.org/onlinepubs/9699919799/functions/stat.html // - Fields here are required for WebAssembly ABI including wasip1 // (a.k.a. wasix) and wasi-filesystem (a.k.a. wasip2). // - This isn't the same as syscall.Stat_t because wazero supports Windows, From c4d684a6c01628f41c590b5e38df273d450ac008 Mon Sep 17 00:00:00 2001 From: Nuno Cruces Date: Mon, 23 Sep 2024 11:46:38 +0100 Subject: [PATCH 3/8] Unused interface. Signed-off-by: Nuno Cruces --- internal/sysfs/file.go | 7 ------- internal/sysfs/osfile.go | 3 --- 2 files changed, 10 deletions(-) diff --git a/internal/sysfs/file.go b/internal/sysfs/file.go index fdbf1fde0d..7aa94ef5f9 100644 --- a/internal/sysfs/file.go +++ b/internal/sysfs/file.go @@ -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 - -// compile-time check to ensure fsFile.reopen implements reopenFile. -var _ reopenFile = (*fsFile)(nil).reopen - // reopen implements the same method as documented on reopenFile. func (f *fsFile) reopen() experimentalsys.Errno { _ = f.close() diff --git a/internal/sysfs/osfile.go b/internal/sysfs/osfile.go index 169d11bcde..08c06ad2b8 100644 --- a/internal/sysfs/osfile.go +++ b/internal/sysfs/osfile.go @@ -91,9 +91,6 @@ func (f *osFile) SetAppend(enable bool) (errno experimentalsys.Errno) { return fileError(f, f.closed, f.reopen()) } -// compile-time check to ensure osFile.reopen implements reopenFile. -var _ reopenFile = (*osFile)(nil).reopen - func (f *osFile) reopen() (errno experimentalsys.Errno) { // Clear any create flag, as we are re-opening, not re-creating. f.flag &= ^experimentalsys.O_CREAT From 15a0dc159bf0478ff433241698f969b66e76080c Mon Sep 17 00:00:00 2001 From: Nuno Cruces Date: Mon, 23 Sep 2024 11:54:52 +0100 Subject: [PATCH 4/8] Check same file. Signed-off-by: Nuno Cruces --- internal/sysfs/file_test.go | 36 ++++++++++++++++++++++++++++++------ internal/sysfs/osfile.go | 26 +++++++++++++++++++++++--- 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/internal/sysfs/file_test.go b/internal/sysfs/file_test.go index 8e69921fc2..9bcd1a2201 100644 --- a/internal/sysfs/file_test.go +++ b/internal/sysfs/file_test.go @@ -120,19 +120,43 @@ func TestFileReopenFileUpdatesFD(t *testing.T) { tmpDir := t.TempDir() path := path.Join(tmpDir, "file") - // Open the file twice. Closing the first file, frees its FD. - // Then reopening the second file will take over the first FD. - // If reopening the file doesn't update the FD, they won't match. + // Open the file twice. Closing the first file, frees its fd. + // Then reopening the second file will take over the first fd. + // If reopening the file doesn't update the fd, they won't match. f0 := requireOpenFile(t, path, experimentalsys.O_RDWR|experimentalsys.O_CREAT, 0o600) - f1 := requireOpenFile(t, path, experimentalsys.O_RDWR|experimentalsys.O_CREAT, 0o600) + f1 := requireOpenFile(t, path, experimentalsys.O_RDWR, 0o600) defer f1.Close() f0.Close() of, ok := f1.(*osFile) require.True(t, ok) - errno := of.reopen() - require.EqualErrno(t, 0, errno) + require.EqualErrno(t, 0, of.reopen()) + require.Equal(t, of.file.Fd(), of.fd) +} + +func TestFileReopenFileChecksSameFile(t *testing.T) { + tmpDir := t.TempDir() + path := path.Join(tmpDir, "file") + + f := requireOpenFile(t, path, experimentalsys.O_RDWR|experimentalsys.O_CREAT, 0o600) + defer f.Close() + + require.NoError(t, os.Remove(path)) + + of, ok := f.(*osFile) + require.True(t, ok) + + // Path does not exist anymore. + require.EqualErrno(t, experimentalsys.ENOENT, of.reopen()) + require.Equal(t, of.file.Fd(), of.fd) + + tmp, err := os.Create(path) + require.NoError(t, err) + defer tmp.Close() + + // Path exists, but is not the same file. + require.EqualErrno(t, experimentalsys.ENOENT, of.reopen()) require.Equal(t, of.file.Fd(), of.fd) } diff --git a/internal/sysfs/osfile.go b/internal/sysfs/osfile.go index 08c06ad2b8..d2f7799c74 100644 --- a/internal/sysfs/osfile.go +++ b/internal/sysfs/osfile.go @@ -113,12 +113,17 @@ func (f *osFile) reopen() (errno experimentalsys.Errno) { } } - _ = f.close() - f.file, errno = OpenFile(f.path, f.flag, f.perm) - f.fd = f.file.Fd() + file, errno := OpenFile(f.path, f.flag, f.perm) + if errno != 0 { + return errno + } + errno = f.checkSameFile(file) if errno != 0 { return errno } + _ = f.close() + f.file = file + f.fd = file.Fd() if !isDir { _, err = f.file.Seek(offset, io.SeekStart) @@ -130,6 +135,21 @@ func (f *osFile) reopen() (errno experimentalsys.Errno) { return 0 } +func (f *osFile) checkSameFile(osf *os.File) experimentalsys.Errno { + fi1, err := f.file.Stat() + if err != nil { + return experimentalsys.UnwrapOSError(err) + } + fi2, err := osf.Stat() + if err != nil { + return experimentalsys.UnwrapOSError(err) + } + if os.SameFile(fi1, fi2) { + return 0 + } + return experimentalsys.ENOENT +} + // IsNonblock implements the same method as documented on fsapi.File func (f *osFile) IsNonblock() bool { return isNonblock(f) From 063b70f01e27a4eb8ec0afc97fb7c497fe03f5bf Mon Sep 17 00:00:00 2001 From: Nuno Cruces Date: Mon, 23 Sep 2024 12:03:52 +0100 Subject: [PATCH 5/8] Docs. Signed-off-by: Nuno Cruces --- internal/sysfs/osfile.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/sysfs/osfile.go b/internal/sysfs/osfile.go index d2f7799c74..e92743a53e 100644 --- a/internal/sysfs/osfile.go +++ b/internal/sysfs/osfile.go @@ -87,7 +87,7 @@ func (f *osFile) SetAppend(enable bool) (errno experimentalsys.Errno) { 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 + // file. https://github.com/golang/go/blob/go1.23/src/os/file_unix.go#L60 return fileError(f, f.closed, f.reopen()) } From 9b288f76de3c2581983fe2f09a378edd383f5585 Mon Sep 17 00:00:00 2001 From: Nuno Cruces Date: Mon, 23 Sep 2024 12:14:49 +0100 Subject: [PATCH 6/8] More. Signed-off-by: Nuno Cruces --- internal/sysfs/osfile.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/internal/sysfs/osfile.go b/internal/sysfs/osfile.go index e92743a53e..e2d89fb2dd 100644 --- a/internal/sysfs/osfile.go +++ b/internal/sysfs/osfile.go @@ -92,9 +92,6 @@ func (f *osFile) SetAppend(enable bool) (errno experimentalsys.Errno) { } func (f *osFile) reopen() (errno experimentalsys.Errno) { - // Clear any create flag, as we are re-opening, not re-creating. - f.flag &= ^experimentalsys.O_CREAT - var ( isDir bool offset int64 @@ -113,7 +110,9 @@ func (f *osFile) reopen() (errno experimentalsys.Errno) { } } - file, errno := OpenFile(f.path, f.flag, f.perm) + // Clear any create flag, as we are re-opening, not re-creating. + flag := f.flag &^ experimentalsys.O_CREAT + file, errno := OpenFile(f.path, flag, f.perm) if errno != 0 { return errno } @@ -121,17 +120,19 @@ func (f *osFile) reopen() (errno experimentalsys.Errno) { if errno != 0 { return errno } - _ = f.close() - f.file = file - f.fd = file.Fd() if !isDir { - _, err = f.file.Seek(offset, io.SeekStart) + _, err = file.Seek(offset, io.SeekStart) if err != nil { + _ = file.Close() return experimentalsys.UnwrapOSError(err) } } + // Only update f on success. + _ = f.file.Close() + f.file = file + f.fd = file.Fd() return 0 } From 81f140ecb9e4dfba37fa32ab32b2683aa54dc54a Mon Sep 17 00:00:00 2001 From: Nuno Cruces Date: Mon, 23 Sep 2024 12:35:19 +0100 Subject: [PATCH 7/8] Never truncate. Signed-off-by: Nuno Cruces --- internal/sysfs/osfile.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/internal/sysfs/osfile.go b/internal/sysfs/osfile.go index e2d89fb2dd..a9b01eb6a9 100644 --- a/internal/sysfs/osfile.go +++ b/internal/sysfs/osfile.go @@ -83,11 +83,8 @@ 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) - - // appendMode (bool) cannot be changed later, so we have to re-open the - // file. https://github.com/golang/go/blob/go1.23/src/os/file_unix.go#L60 + // appendMode cannot be changed later, so we have to re-open the file + // https://github.com/golang/go/blob/go1.23/src/os/file_unix.go#L60 return fileError(f, f.closed, f.reopen()) } @@ -110,8 +107,8 @@ func (f *osFile) reopen() (errno experimentalsys.Errno) { } } - // Clear any create flag, as we are re-opening, not re-creating. - flag := f.flag &^ experimentalsys.O_CREAT + // Clear any create or trunc flag, as we are re-opening, not re-creating. + flag := f.flag &^ (experimentalsys.O_CREAT | experimentalsys.O_TRUNC) file, errno := OpenFile(f.path, flag, f.perm) if errno != 0 { return errno From 01d74313b5ad4e6de9fddece22cde1cf47588906 Mon Sep 17 00:00:00 2001 From: Nuno Cruces Date: Mon, 23 Sep 2024 12:35:49 +0100 Subject: [PATCH 8/8] Only reopen dirs. Signed-off-by: Nuno Cruces --- internal/sysfs/file.go | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/internal/sysfs/file.go b/internal/sysfs/file.go index 7aa94ef5f9..1b6d5f3dec 100644 --- a/internal/sysfs/file.go +++ b/internal/sysfs/file.go @@ -269,7 +269,7 @@ func (f *fsFile) Readdir(n int) (dirents []experimentalsys.Dirent, errno experim if f.reopenDir { // re-open the directory if needed. f.reopenDir = false - if errno = adjustReaddirErr(f, f.closed, f.reopen()); errno != 0 { + if errno = adjustReaddirErr(f, f.closed, f.rewindDir()); errno != 0 { return } } @@ -418,12 +418,25 @@ func seek(s io.Seeker, offset int64, whence int) (int64, experimentalsys.Errno) return newOffset, experimentalsys.UnwrapOSError(err) } -// reopen implements the same method as documented on reopenFile. -func (f *fsFile) reopen() experimentalsys.Errno { - _ = f.close() - var err error - f.file, err = f.fs.Open(f.name) - return experimentalsys.UnwrapOSError(err) +func (f *fsFile) rewindDir() experimentalsys.Errno { + // Reopen the directory to rewind it. + file, err := f.fs.Open(f.name) + if err != nil { + return experimentalsys.UnwrapOSError(err) + } + fi, err := file.Stat() + if err != nil { + return experimentalsys.UnwrapOSError(err) + } + // Can't check if it's still the same file, + // but is it still a directory, at least? + if !fi.IsDir() { + return experimentalsys.ENOTDIR + } + // Only update f on success. + _ = f.file.Close() + f.file = file + return 0 } // readdirFile allows masking the `Readdir` function on os.File.