From c1edb433849dd29837a94510ea4ad1a92534b663 Mon Sep 17 00:00:00 2001 From: Pavel Koneski Date: Wed, 11 Dec 2024 13:09:49 -0800 Subject: [PATCH 1/4] Use dual stream for os.open with O_APPEND --- Src/IronPython.Modules/nt.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/Src/IronPython.Modules/nt.cs b/Src/IronPython.Modules/nt.cs index 8d91d5438..c5ac5b3c1 100644 --- a/Src/IronPython.Modules/nt.cs +++ b/Src/IronPython.Modules/nt.cs @@ -877,8 +877,9 @@ public static object open(CodeContext/*!*/ context, [NotNone] string path, int f FileMode fileMode = FileModeFromFlags(flags); FileAccess access = FileAccessFromFlags(flags); FileOptions options = FileOptionsFromFlags(flags); - Stream s; - FileStream? fs; + Stream s; // the stream opened to acces the file + FileStream? fs; // downcast of s if s is FileStream (this is always the case on POSIX) + Stream? rs = null; // secondary read stream if needed, otherwise same as s if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && IsNulFile(path)) { fs = null; s = Stream.Null; @@ -889,15 +890,20 @@ public static object open(CodeContext/*!*/ context, [NotNone] string path, int f fs.Close(); s = fs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite, DefaultBufferSize, options); } else if (access == FileAccess.ReadWrite && fileMode == FileMode.Append) { + // .NET doesn't allow Append w/ access != Write, so open the file w/ Write + // and a secondary stream w/ Read, then seek to the end. s = fs = new FileStream(path, FileMode.Append, FileAccess.Write, FileShare.ReadWrite, DefaultBufferSize, options); + rs = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite, DefaultBufferSize, options); + rs.Seek(0L, SeekOrigin.End); } else { s = fs = new FileStream(path, fileMode, access, FileShare.ReadWrite, DefaultBufferSize, options); } + rs ??= s; - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { - return context.LanguageContext.FileManager.Add(new(s)); + if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) { + return context.LanguageContext.FileManager.Add(fs!.SafeFileHandle.DangerousGetHandle().ToInt32(), new(rs, s)); } else { - return context.LanguageContext.FileManager.Add((int)fs!.SafeFileHandle.DangerousGetHandle(), new(s)); + return context.LanguageContext.FileManager.Add(new(rs, s)); } } catch (Exception e) { throw ToPythonException(e, path); From a1e894e87afac6e6f1f31c1e7521e98af72a0440 Mon Sep 17 00:00:00 2001 From: Pavel Koneski Date: Wed, 11 Dec 2024 19:38:03 -0800 Subject: [PATCH 2/4] Maintain file position when accessing file descriptor --- Src/IronPython.Modules/nt.cs | 34 ++++++++++++--------- Src/IronPython/Modules/_fileio.cs | 13 ++++++++ Src/IronPython/Runtime/PythonFileManager.cs | 10 +++--- 3 files changed, 38 insertions(+), 19 deletions(-) diff --git a/Src/IronPython.Modules/nt.cs b/Src/IronPython.Modules/nt.cs index c5ac5b3c1..5ef569320 100644 --- a/Src/IronPython.Modules/nt.cs +++ b/Src/IronPython.Modules/nt.cs @@ -352,10 +352,6 @@ public static int dup(CodeContext/*!*/ context, int fd) { StreamBox streams = fileManager.GetStreams(fd); // OSError if fd not valid if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) { - if (!streams.IsSingleStream && fd is 1 or 2) { - // If there is a separate write stream, dupping over stout or sderr uses write stream's file descriptor - fd = streams.WriteStream is FileStream fs ? fs.SafeFileHandle.DangerousGetHandle().ToInt32() : fd; - } int fd2 = UnixDup(fd, -1, out Stream? dupstream); if (dupstream is not null) { return fileManager.Add(fd2, new(dupstream)); @@ -389,16 +385,14 @@ public static int dup2(CodeContext/*!*/ context, int fd, int fd2) { close(context, fd2); } - // TODO: race condition: `open` or `dup` on another thread may occupy fd2 (simulated descriptors only) + // TODO: race condition: `open` or `dup` on another thread may occupy fd2 - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { - fileManager.EnsureRefStreams(streams); - fileManager.AddRefStreams(streams); - return fileManager.Add(fd2, new(streams)); - } else { - if (!streams.IsSingleStream && fd is 1 or 2) { - // If there is a separate write stream, dupping over stout or sderr uses write stream's file descriptor - fd = streams.WriteStream is FileStream fs ? fs.SafeFileHandle.DangerousGetHandle().ToInt32() : fd; + if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) { + if (!streams.IsSingleStream && fd2 is 0 && streams.ReadStream is FileStream fs) { + // If there is a separate read stream, dupping over stdin uses read stream's file descriptor as source + long pos = fs.Position; + fd = fs.SafeFileHandle.DangerousGetHandle().ToInt32(); + fs.Seek(pos, SeekOrigin.Begin); } fd2 = UnixDup(fd, fd2, out Stream? dupstream); // closes fd2 atomically if reopened in the meantime fileManager.Remove(fd2); @@ -410,6 +404,10 @@ public static int dup2(CodeContext/*!*/ context, int fd, int fd2) { fileManager.AddRefStreams(streams); return fileManager.Add(fd2, new(streams)); } + } else { + fileManager.EnsureRefStreams(streams); + fileManager.AddRefStreams(streams); + return fileManager.Add(fd2, new(streams)); } } @@ -901,7 +899,15 @@ public static object open(CodeContext/*!*/ context, [NotNone] string path, int f rs ??= s; if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) { - return context.LanguageContext.FileManager.Add(fs!.SafeFileHandle.DangerousGetHandle().ToInt32(), new(rs, s)); + int fd = fs!.SafeFileHandle.DangerousGetHandle().ToInt32(); + // accessing SafeFileHandle may reset file position + if (fileMode == FileMode.Append) { + fs.Seek(0L, SeekOrigin.End); + } + if (!ReferenceEquals(fs, rs)) { + rs.Seek(fs.Position, SeekOrigin.Begin); + } + return context.LanguageContext.FileManager.Add(fd, new(rs, s)); } else { return context.LanguageContext.FileManager.Add(new(rs, s)); } diff --git a/Src/IronPython/Modules/_fileio.cs b/Src/IronPython/Modules/_fileio.cs index 93e2135bf..86ffa1c9c 100644 --- a/Src/IronPython/Modules/_fileio.cs +++ b/Src/IronPython/Modules/_fileio.cs @@ -127,6 +127,19 @@ public FileIO(CodeContext/*!*/ context, string name, string mode = "r", bool clo default: throw new InvalidOperationException(); } + if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) { + // On POSIX, register the file descriptor with the file manager right after file opening + _context.FileManager.GetOrAssignId(_streams); + // accoding to [documentation](https://learn.microsoft.com/en-us/dotnet/api/system.io.filestream.safefilehandle?view=net-9.0#remarks) + // accessing SafeFileHandle sets the current stream position to 0 + // in practice it doesn't seem to be the case, but better to be sure + if (this.mode == "ab+") { + _streams.WriteStream.Seek(0L, SeekOrigin.End); + } + if (!_streams.IsSingleStream) { + _streams.ReadStream.Seek(_streams.WriteStream.Position, SeekOrigin.Begin); + } + } } else { object fdobj = PythonOps.CallWithContext(context, opener, name, flags); diff --git a/Src/IronPython/Runtime/PythonFileManager.cs b/Src/IronPython/Runtime/PythonFileManager.cs index 64a81bb9b..c98811608 100644 --- a/Src/IronPython/Runtime/PythonFileManager.cs +++ b/Src/IronPython/Runtime/PythonFileManager.cs @@ -287,12 +287,12 @@ public int GetOrAssignId(StreamBox streams) { lock (_synchObject) { int res = streams.Id; if (res == -1) { - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { - res = Add(streams); - } else { - res = GetGenuineFileDescriptor(streams.ReadStream); + if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) { + res = GetGenuineFileDescriptor(streams.WriteStream); if (res < 0) throw new InvalidOperationException("stream not associated with a file descriptor"); Add(res, streams); + } else { + res = Add(streams); } } return res; @@ -327,7 +327,7 @@ public bool ValidateFdRange(int fd) { return fd >= 0 && fd < LIMIT_OFILE; } - [UnsupportedOSPlatform("windows")] + [SupportedOSPlatform("linux"), SupportedOSPlatform("macos")] private static int GetGenuineFileDescriptor(Stream stream) { return stream switch { FileStream fs => checked((int)fs.SafeFileHandle.DangerousGetHandle()), From d51c7a5d4e3641659695c34e5ed295424c5ae327 Mon Sep 17 00:00:00 2001 From: Pavel Koneski Date: Wed, 11 Dec 2024 20:44:07 -0800 Subject: [PATCH 3/4] Add test --- Tests/modules/system_related/test_os.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/Tests/modules/system_related/test_os.py b/Tests/modules/system_related/test_os.py index 24784fd4a..d27c6dece 100644 --- a/Tests/modules/system_related/test_os.py +++ b/Tests/modules/system_related/test_os.py @@ -7,6 +7,14 @@ from iptest import IronPythonTestCase, is_osx, is_linux, is_windows, run_test class OsTest(IronPythonTestCase): + def setUp(self): + super(OsTest, self).setUp() + self.temp_file = os.path.join(self.temporary_dir, "temp_OSTest_%d.dat" % os.getpid()) + + def tearDown(self): + self.delete_files(self.temp_file) + return super().tearDown() + def test_strerror(self): if is_windows: self.assertEqual(os.strerror(0), "No error") @@ -29,4 +37,20 @@ def test_strerror(self): elif is_osx: self.assertEqual(os.strerror(40), "Message too long") + def test_open_abplus(self): + # equivalent to open(self.temp_file, "ab+"), see also test_file.test_open_abplus + fd = os.open(self.temp_file, os.O_APPEND | os.O_CREAT | os.O_RDWR) + try: + f = open(fd, mode="ab+", closefd=False) + f.write(b"abc") + f.seek(0) + self.assertEqual(f.read(2), b"ab") + f.write(b"def") + self.assertEqual(f.read(2), b"") + f.seek(0) + self.assertEqual(f.read(6), b"abcdef") + f.close() + finally: + os.close(fd) + run_test(__name__) From 941e3c25e61b323f41c7502eb69b279c27e154f5 Mon Sep 17 00:00:00 2001 From: Pavel Koneski Date: Thu, 12 Dec 2024 20:46:19 -0800 Subject: [PATCH 4/4] Fix typo --- Src/IronPython/Modules/_fileio.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Src/IronPython/Modules/_fileio.cs b/Src/IronPython/Modules/_fileio.cs index 86ffa1c9c..700a6691a 100644 --- a/Src/IronPython/Modules/_fileio.cs +++ b/Src/IronPython/Modules/_fileio.cs @@ -130,7 +130,7 @@ public FileIO(CodeContext/*!*/ context, string name, string mode = "r", bool clo if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) { // On POSIX, register the file descriptor with the file manager right after file opening _context.FileManager.GetOrAssignId(_streams); - // accoding to [documentation](https://learn.microsoft.com/en-us/dotnet/api/system.io.filestream.safefilehandle?view=net-9.0#remarks) + // according to [documentation](https://learn.microsoft.com/en-us/dotnet/api/system.io.filestream.safefilehandle?view=net-9.0#remarks) // accessing SafeFileHandle sets the current stream position to 0 // in practice it doesn't seem to be the case, but better to be sure if (this.mode == "ab+") {