From cd5c9ed37cd311a1a68fee13e91b3248225c7def Mon Sep 17 00:00:00 2001 From: Crypt Keeper <64215+codefromthecrypt@users.noreply.github.com> Date: Fri, 12 May 2023 14:35:17 +0800 Subject: [PATCH] Clears O_CREATE when re-opening a file to set flags (#1458) Signed-off-by: Adrian Cole --- imports/wasi_snapshot_preview1/fs.go | 2 +- imports/wasi_snapshot_preview1/fs_test.go | 26 ++++++++++++++--------- internal/sys/fs.go | 10 +++++++++ internal/sys/fs_test.go | 18 +++++++++++++++- 4 files changed, 44 insertions(+), 12 deletions(-) diff --git a/imports/wasi_snapshot_preview1/fs.go b/imports/wasi_snapshot_preview1/fs.go index ba7f9521..44543942 100644 --- a/imports/wasi_snapshot_preview1/fs.go +++ b/imports/wasi_snapshot_preview1/fs.go @@ -205,7 +205,7 @@ func fdFdstatGetFn(_ context.Context, mod api.Module, params []uint64) syscall.E if f.File.IsNonblock() { fdflags |= wasip1.FD_NONBLOCK } - } else if f.File.AccessMode() != syscall.O_RDONLY { + } else if f.IsAppend() { fdflags |= wasip1.FD_APPEND } diff --git a/imports/wasi_snapshot_preview1/fs_test.go b/imports/wasi_snapshot_preview1/fs_test.go index 29bacc0a..fd4d2b83 100644 --- a/imports/wasi_snapshot_preview1/fs_test.go +++ b/imports/wasi_snapshot_preview1/fs_test.go @@ -250,7 +250,8 @@ func Test_fdFdstatGet(t *testing.T) { stdin.File = stdinFile - fileFD, errno := fsc.OpenFile(preopen, file, os.O_RDONLY, 0) + // Make this file writeable, to ensure flags read-back correctly. + fileFD, errno := fsc.OpenFile(preopen, file, os.O_RDWR|os.O_APPEND, 0) require.EqualErrno(t, 0, errno) dirFD, errno := fsc.OpenFile(preopen, dir, os.O_RDONLY, 0) @@ -325,13 +326,13 @@ func Test_fdFdstatGet(t *testing.T) { fd: fileFD, expectedMemory: []byte{ 4, 0, // fs_filetype - 0, 0, 0, 0, 0, 0, // fs_flags + 1, 0, 0, 0, 0, 0, // fs_flags 0xff, 0x1, 0xe0, 0x8, 0x0, 0x0, 0x0, 0x0, // fs_rights_base 0, 0, 0, 0, 0, 0, 0, 0, // fs_rights_inheriting }, expectedLog: ` ==> wasi_snapshot_preview1.fd_fdstat_get(fd=4) -<== (stat={filetype=REGULAR_FILE,fdflags=,fs_rights_base=FD_DATASYNC|FD_READ|FD_SEEK|FDSTAT_SET_FLAGS|FD_SYNC|FD_TELL|FD_WRITE|FD_ADVISE|FD_ALLOCATE,fs_rights_inheriting=},errno=ESUCCESS) +<== (stat={filetype=REGULAR_FILE,fdflags=APPEND,fs_rights_base=FD_DATASYNC|FD_READ|FD_SEEK|FDSTAT_SET_FLAGS|FD_SYNC|FD_TELL|FD_WRITE|FD_ADVISE|FD_ALLOCATE,fs_rights_inheriting=},errno=ESUCCESS) `, }, { @@ -481,11 +482,6 @@ func Test_fdFdstatGet_StdioNonblock(t *testing.T) { func Test_fdFdstatSetFlags(t *testing.T) { tmpDir := t.TempDir() // open before loop to ensure no locking problems. - const fileName = "file.txt" - - // Create the target file. - realPath := joinPath(tmpDir, fileName) - require.NoError(t, os.WriteFile(realPath, []byte("0123456789"), 0o600)) stdinR, stdinW := openPipe(t) defer closePipe(stdinR, stdinW) @@ -505,8 +501,18 @@ func Test_fdFdstatSetFlags(t *testing.T) { preopen := fsc.RootFS() defer r.Close(testCtx) - // First, open it with O_APPEND. - fd, errno := fsc.OpenFile(preopen, fileName, os.O_RDWR|os.O_APPEND, 0) + // First, O_CREATE the file with O_APPEND. We use O_EXCL because that + // triggers an EEXIST error if called a second time with O_CREATE. Our + // logic should clear O_CREATE preventing this. + const fileName = "file.txt" + // Create the target file. + fd, errno := fsc.OpenFile(preopen, fileName, os.O_RDWR|os.O_APPEND|os.O_CREATE|syscall.O_EXCL, 0o600) + require.EqualErrno(t, 0, errno) + + // Write the initial text to the file. + f, ok := fsc.LookupFile(fd) + require.True(t, ok) + _, errno = f.File.Write([]byte("0123456789")) require.EqualErrno(t, 0, errno) writeWazero := func() { diff --git a/internal/sys/fs.go b/internal/sys/fs.go index 4f7c0c5c..d9b0d66e 100644 --- a/internal/sys/fs.go +++ b/internal/sys/fs.go @@ -255,6 +255,11 @@ type cachedStat struct { Ino uint64 } +// IsAppend returns true if the file is open with syscall.O_APPEND. +func (f *FileEntry) IsAppend() bool { + return f.openFlag&syscall.O_APPEND != 0 +} + // Inode returns the cached inode from of platform.Stat_t or an error if it // couldn't be retrieved. func (f *FileEntry) Inode() (ino uint64, errno syscall.Errno) { @@ -462,6 +467,8 @@ func (c *FSContext) ChangeOpenFlag(fd int32, flag int) syscall.Errno { return errno } else if isDir { return syscall.EISDIR + } else if flag&syscall.O_APPEND == f.openFlag&syscall.O_APPEND { + return 0 // don't re-open } if flag&syscall.O_APPEND != 0 { @@ -470,6 +477,9 @@ func (c *FSContext) ChangeOpenFlag(fd int32, flag int) syscall.Errno { f.openFlag &= ^syscall.O_APPEND } + // Clear any create flag, as we are re-opening, not re-creating. + f.openFlag &= ^syscall.O_CREAT + // Changing the flag while opening is not really supported well in Go. Even when using // syscall package, the feasibility of doing so really depends on the platform. For examples: // diff --git a/internal/sys/fs_test.go b/internal/sys/fs_test.go index 59839630..36dd7876 100644 --- a/internal/sys/fs_test.go +++ b/internal/sys/fs_test.go @@ -329,7 +329,7 @@ func TestFSContext_ChangeOpenFlag(t *testing.T) { tmpDir := t.TempDir() dirFs := sysfs.NewDirFS(tmpDir) - const fileName = "dir" + const fileName = "file" require.NoError(t, os.WriteFile(path.Join(tmpDir, fileName), []byte("0123456789"), 0o600)) c := Context{} @@ -360,6 +360,22 @@ func TestFSContext_ChangeOpenFlag(t *testing.T) { f2, ok := fsc.openedFiles.Lookup(fd) require.True(t, ok) require.Equal(t, f2.openFlag&syscall.O_APPEND, 0) + + t.Run("create exclusive", func(t *testing.T) { + // O_EXCL triggers an EEXIST error if called a second time with + // O_CREATE. This test proves the internal logic clears O_CREATE on + // re-open. + const fileName = "exclusive" + fd, errno := fsc.OpenFile(dirFs, fileName, os.O_RDWR|os.O_CREATE|syscall.O_EXCL, 0o600) + require.EqualErrno(t, 0, errno) + + errno = fsc.ChangeOpenFlag(fd, syscall.O_APPEND) + require.EqualErrno(t, 0, errno) + + f1, ok := fsc.openedFiles.Lookup(fd) + require.True(t, ok) + require.Equal(t, f1.openFlag&syscall.O_APPEND, syscall.O_APPEND) + }) } func TestStdio(t *testing.T) {