Skip to content

Isolate POSIX-specific native calls in PythonNT into separate file #1870

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
merged 7 commits into from
Jan 13, 2025
Merged

Isolate POSIX-specific native calls in PythonNT into separate file #1870

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fcc8db8
Isolate POSIX-specific native calls in PythonNT into separate file
BCSharp Jan 10, 2025
9035d36
Simplify error creation
BCSharp Jan 10, 2025
17eb367
Clean up some error usage in mmap
BCSharp Jan 11, 2025
4ff3b33
Normalize macos platform name
BCSharp Jan 11, 2025
ead66d5
Fix some mmap errors on POSIX
BCSharp Jan 11, 2025
7113ba3
Move more functions to posix.cs
BCSharp Jan 13, 2025
6e409b5
Update after review
BCSharp Jan 13, 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
3 changes: 3 additions & 0 deletions Src/IronPython.Modules/_overlapped.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,14 @@
using System;
using System.Numerics;
using System.Runtime.InteropServices;
using System.Runtime.Versioning;

using IronPython.Runtime;

[assembly: PythonModule("_overlapped", typeof(IronPython.Modules.PythonOverlapped), PlatformsAttribute.PlatformFamily.Windows)]
namespace IronPython.Modules {

[SupportedOSPlatform("windows")]
public static class PythonOverlapped {
public const int ERROR_NETNAME_DELETED = 64;
public const int ERROR_SEM_TIMEOUT = 121;
Expand Down
3 changes: 3 additions & 0 deletions Src/IronPython.Modules/_winapi.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,19 @@
using System;
using System.Numerics;
using System.Runtime.InteropServices;
using System.Runtime.Versioning;
using System.Text;

using IronPython.Runtime;
using IronPython.Runtime.Operations;

[assembly: PythonModule("_winapi", typeof(IronPython.Modules.PythonWinApi), PlatformsAttribute.PlatformFamily.Windows)]
namespace IronPython.Modules {
[SupportedOSPlatform("windows")]
public static class PythonWinApi {
#region Public API

[SupportedOSPlatform("windows")]
Copy link
Contributor

@slozier slozier Jan 13, 2025

Choose a reason for hiding this comment

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

Do we need the attribute on both the class and method?

Copy link
Member Author

@BCSharp BCSharp Jan 13, 2025

Choose a reason for hiding this comment

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

No, it's redundant.

public static object? ConnectNamedPipe(BigInteger handle, bool overlapped = false) {
if (overlapped) throw new NotImplementedException();

Expand Down
35 changes: 6 additions & 29 deletions Src/IronPython.Modules/mmap.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,13 +339,13 @@ public MmapDefault(CodeContext/*!*/ context, int fileno, long length, string tag
#if NET8_0_OR_GREATER
// On .NET 8.0+ we can create a MemoryMappedFile directly from a file descriptor
stream.Flush();
CheckFileAccessAndSize(stream);
fileno = Dup(fileno);
CheckFileAccessAndSize(stream, isWindows: false);
fileno = PythonNT.dupUnix(fileno, closeOnExec: true);
_handle = new SafeFileHandle((IntPtr)fileno, ownsHandle: true);
_file = MemoryMappedFile.CreateFromFile(_handle, _mapName, stream.Length, _fileAccess, HandleInheritability.None, leaveOpen: true);
#else
// On .NET 6.0 on POSIX we need to create a FileStream from the file descriptor
fileno = Dup(fileno);
fileno = PythonNT.dupUnix(fileno, closeOnExec: true);
_handle = new SafeFileHandle((IntPtr)fileno, ownsHandle: true);
FileAccess fa = stream.CanWrite ? stream.CanRead ? FileAccess.ReadWrite : FileAccess.Write : FileAccess.Read;
// This FileStream constructor may or may not work on Mono, but on Mono streams.ReadStream is FileStream
Expand All @@ -369,7 +369,7 @@ public MmapDefault(CodeContext/*!*/ context, int fileno, long length, string tag
length = _sourceStream.Length - _offset;
}

CheckFileAccessAndSize(_sourceStream);
CheckFileAccessAndSize(_sourceStream, RuntimeInformation.IsOSPlatform(OSPlatform.Windows));

long capacity = checked(_offset + length);

Expand Down Expand Up @@ -402,7 +402,7 @@ public MmapDefault(CodeContext/*!*/ context, int fileno, long length, string tag
}
_position = 0L;

void CheckFileAccessAndSize(Stream stream) {
void CheckFileAccessAndSize(Stream stream, bool isWindows) {
bool isValid = _fileAccess switch {
MemoryMappedFileAccess.Read => stream.CanRead,
MemoryMappedFileAccess.ReadWrite => stream.CanRead && stream.CanWrite,
Expand All @@ -417,7 +417,7 @@ void CheckFileAccessAndSize(Stream stream) {
throw PythonOps.OSError(PythonExceptions._OSError.ERROR_ACCESS_DENIED, "Invalid access mode");
}

if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
if (!isWindows) {
Copy link
Contributor

@slozier slozier Jan 10, 2025

Choose a reason for hiding this comment

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

Not really an issue since we're not calling native APIs here, but any idea if the platform analyzer would pick up on an isWindows variable and guard against Windows calls?

Copy link
Member Author

@BCSharp BCSharp Jan 10, 2025

Choose a reason for hiding this comment

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

As it is written now, no platform-specific calls are allowed in CheckFileAccessAndSize, regardless whether they are conditioned by isWindows in any way. .NET has SupportedOSPlatformGuardAttribute, but it cannot be applied to a parameter.

The reason why I replaced the platform check with a parameter is because having the platform test in this function was invalidating the platform context after the first call site, and I needed to make a call to PythonNT.dupUnix.

Copy link
Contributor

@slozier slozier Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Cool, didn't know (or forgot) that SupportedOSPlatformGuardAttribute existed.

Weird that iy breaks the platform check. Alternatively the parameter could be called allowGrow or something.

// Unix map does not support increasing size on open
if (length != 0 && _offset + length > stream.Length) {
throw PythonOps.ValueError("mmap length is greater than file size");
Expand All @@ -437,29 +437,6 @@ void CheckFileAccessAndSize(Stream stream) {
} // end of constructor


// TODO: Move to PythonNT - POSIX
private static int Dup(int fd) {
int fd2 = Mono.Unix.Native.Syscall.dup(fd);
if (fd2 == -1) throw PythonNT.GetLastUnixError();

try {
// set close-on-exec flag
int flags = Mono.Unix.Native.Syscall.fcntl(fd2, Mono.Unix.Native.FcntlCommand.F_GETFD);
if (flags == -1) throw PythonNT.GetLastUnixError();

const int FD_CLOEXEC = 1; // TODO: Move to module fcntl
flags |= FD_CLOEXEC;
flags = Mono.Unix.Native.Syscall.fcntl(fd2, Mono.Unix.Native.FcntlCommand.F_SETFD, flags);
if (flags == -1) throw PythonNT.GetLastUnixError();
} catch {
Mono.Unix.Native.Syscall.close(fd2);
throw;
}

return fd2;
}


public object __len__() {
using (new MmapLocker(this)) {
return ReturnLong(_view.Capacity);
Expand Down
147 changes: 21 additions & 126 deletions Src/IronPython.Modules/nt.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@

[assembly: PythonModule("nt", typeof(IronPython.Modules.PythonNT))]
namespace IronPython.Modules {
public static class PythonNT {
public static partial class PythonNT {
public const string __doc__ = "Provides low-level operating system access for files, the environment, etc...";

/* TODO: missing functions/classes:
Expand Down Expand Up @@ -301,12 +301,6 @@ public static void chdir(CodeContext context, [NotNone] Bytes path)
public static void chdir(CodeContext context, object? path)
=> chdir(ConvertToFsString(context, path, nameof(path)));

// Isolate Mono.Unix from the rest of the method so that we don't try to load the Mono.Unix assembly on Windows.
private static void chmodUnix(string path, int mode) {
if (Mono.Unix.Native.Syscall.chmod(path, Mono.Unix.Native.NativeConvert.ToFilePermissions((uint)mode)) == 0) return;
throw GetLastUnixError(path);
}

[Documentation("chmod(path, mode, *, dir_fd=None, follow_symlinks=True)")]
public static void chmod([NotNone] string path, int mode, [ParamDictionary, NotNone] IDictionary<string, object> kwargs) {
foreach (var key in kwargs.Keys) {
Expand Down Expand Up @@ -371,7 +365,7 @@ 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)) {
int fd2 = UnixDup(fd, -1, out Stream? dupstream);
int fd2 = DuplicateStreamDescriptorUnix(fd, -1, out Stream? dupstream);
if (dupstream is not null) {
return fileManager.Add(fd2, new(dupstream));
} else {
Expand Down Expand Up @@ -413,7 +407,7 @@ public static int dup2(CodeContext/*!*/ context, int fd, int fd2) {
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
fd2 = DuplicateStreamDescriptorUnix(fd, fd2, out Stream? dupstream); // closes fd2 atomically if reopened in the meantime
fileManager.Remove(fd2);
if (dupstream is not null) {
return fileManager.Add(fd2, new(dupstream));
Expand All @@ -431,41 +425,6 @@ public static int dup2(CodeContext/*!*/ context, int fd, int fd2) {
}


[SupportedOSPlatform("linux"), SupportedOSPlatform("osx")]
private static int UnixDup(int fd, int fd2, out Stream? stream) {
int res = fd2 < 0 ? Mono.Unix.Native.Syscall.dup(fd) : Mono.Unix.Native.Syscall.dup2(fd, fd2);
if (res < 0) throw GetLastUnixError();
if (ClrModule.IsMono) {
// Elaborate workaround on Mono to avoid UnixStream as out
stream = new Mono.Unix.UnixStream(res, ownsHandle: false);
FileAccess fileAccess = stream.CanWrite ? stream.CanRead ? FileAccess.ReadWrite : FileAccess.Write : FileAccess.Read;
stream.Dispose();
try {
// FileStream on Mono created with a file descriptor might not work: https://github.com/mono/mono/issues/12783
// Test if it does, without closing the handle if it doesn't
var sfh = new SafeFileHandle((IntPtr)res, ownsHandle: false);
stream = new FileStream(sfh, fileAccess);
// No exception? Great! We can use FileStream.
stream.Dispose();
sfh.Dispose();
stream = null; // Create outside of try block
} catch (IOException) {
// Fall back to UnixStream
stream = new Mono.Unix.UnixStream(res, ownsHandle: true);
}
if (stream is null) {
// FileStream is safe
var sfh = new SafeFileHandle((IntPtr)res, ownsHandle: true);
stream = new FileStream(sfh, fileAccess);
}
} else {
// normal case
stream = new PosixFileStream(res);
}
return res;
}


#if FEATURE_PROCESS
/// <summary>
/// single instance of environment dictionary is shared between multiple runtimes because the environment
Expand Down Expand Up @@ -563,11 +522,6 @@ static void linkWindows(string src, string dst) {
if (!CreateHardLink(dst, src, IntPtr.Zero))
throw GetLastWin32Error(src, dst);
}

static void linkUnix(string src, string dst) {
if (Mono.Unix.Native.Syscall.link(src, dst) == 0) return;
throw GetLastUnixError(src, dst);
}
}

public static bool isatty(CodeContext context, int fd) {
Expand Down Expand Up @@ -779,11 +733,6 @@ public static void symlink([NotNone] string src, [NotNone] string dst, [ParamDic
} else {
throw new NotImplementedException();
}

static void symlinkUnix(string src, string dst) {
if (Mono.Unix.Native.Syscall.symlink(src, dst) == 0) return;
throw GetLastUnixError(src, dst);
}
}

[Documentation("")]
Expand Down Expand Up @@ -1012,17 +961,14 @@ public static PythonTuple pipe(CodeContext context) {
manager.Add(new(inPipe)),
manager.Add(new(outPipe))
);
} else {
} else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
var pipeStreams = CreatePipeStreamsUnix();
return PythonTuple.MakeTuple(
manager.Add(pipeStreams.Item1, new(pipeStreams.Item2)),
manager.Add(pipeStreams.Item3, new(pipeStreams.Item4))
);
}

static Tuple<int, Stream, int, Stream> CreatePipeStreamsUnix() {
Mono.Unix.UnixPipes pipes = Mono.Unix.UnixPipes.CreatePipes();
return Tuple.Create<int, Stream, int, Stream>(pipes.Reading.Handle, pipes.Reading, pipes.Writing.Handle, pipes.Writing);
} else {
throw new PlatformNotSupportedException();
}
}
#endif
Expand Down Expand Up @@ -1094,11 +1040,6 @@ public static void rename(CodeContext context, object? src, object? dst, [ParamD
[DllImport("kernel32.dll", EntryPoint = "MoveFileExW", SetLastError = true, CharSet = CharSet.Unicode, BestFitMapping = false)]
private static extern bool MoveFileEx(string src, string dst, uint flags);

private static void renameUnix(string src, string dst) {
if (Mono.Unix.Native.Syscall.rename(src, dst) == 0) return;
throw GetLastUnixError(src, dst);
}

[Documentation("replace(src, dst, *, src_dir_fd=None, dst_dir_fd=None)")]
public static void replace([NotNone] string src, [NotNone] string dst, [ParamDictionary, NotNone] IDictionary<string, object> kwargs) {
foreach (var key in kwargs.Keys) {
Expand Down Expand Up @@ -1467,21 +1408,6 @@ private static bool HasExecutableExtension(string path) {
return (extension == ".exe" || extension == ".dll" || extension == ".com" || extension == ".bat");
}

// Isolate Mono.Unix from the rest of the method so that we don't try to load the Mono.Unix assembly on Windows.
private static object statUnix(string path) {
if (Mono.Unix.Native.Syscall.stat(path, out Mono.Unix.Native.Stat buf) == 0) {
return new stat_result(buf);
}
return LightExceptions.Throw(GetLastUnixError(path));
}

private static object fstatUnix(int fd) {
if (Mono.Unix.Native.Syscall.fstat(fd, out Mono.Unix.Native.Stat buf) == 0) {
return new stat_result(buf);
}
return LightExceptions.Throw(GetLastUnixError());
}

private const int OPEN_EXISTING = 3;
private const int FILE_ATTRIBUTE_NORMAL = 0x00000080;
private const int FILE_READ_ATTRIBUTES = 0x0080;
Expand Down Expand Up @@ -1616,9 +1542,12 @@ public static string strerror(int code) {
const int bufsize = 0x1FF;
var buffer = new StringBuilder(bufsize);

int result = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ?
Interop.Ucrtbase.strerror(code, buffer) :
strerror_r(code, buffer);
int result = -1;
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) {
result = Interop.Ucrtbase.strerror(code, buffer);
} else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
result = strerror_r(code, buffer);
}

if (result == 0) {
var msg = buffer.ToString();
Expand All @@ -1630,12 +1559,6 @@ public static string strerror(int code) {
return "Unknown error " + code;
}

#if FEATURE_NATIVE
// Isolate Mono.Unix from the rest of the method so that we don't try to load the Mono.Unix assembly on Windows.
private static int strerror_r(int code, StringBuilder buffer)
=> Mono.Unix.Native.Syscall.strerror_r(Mono.Unix.Native.NativeConvert.ToErrno(code), buffer);
#endif

#if FEATURE_PROCESS
[Documentation("system(command) -> int\nExecute the command (a string) in a subshell.")]
public static int system([NotNone] string command) {
Expand Down Expand Up @@ -1724,18 +1647,6 @@ public static void ftruncate(CodeContext context, int fd, BigInteger length) {
}


[SupportedOSPlatform("linux"), SupportedOSPlatform("osx")]
internal static void ftruncateUnix(int fd, long length) {
int result;
Mono.Unix.Native.Errno errno;
do {
result = Mono.Unix.Native.Syscall.ftruncate(fd, length);
} while (Mono.Unix.UnixMarshal.ShouldRetrySyscall(result, out errno));

if (errno != 0)
throw GetOsError(Mono.Unix.Native.NativeConvert.FromErrno(errno));
}


#if FEATURE_FILESYSTEM
public static object times() {
Expand Down Expand Up @@ -1841,18 +1752,6 @@ public static int umask(CodeContext/*!*/ context, object? mask)

#if FEATURE_FILESYSTEM

private static void utimeUnix(string path, long atime_ns, long utime_ns) {
var atime = new Mono.Unix.Native.Timespec();
atime.tv_sec = atime_ns / 1_000_000_000;
atime.tv_nsec = atime_ns % 1_000_000_000;
var utime = new Mono.Unix.Native.Timespec();
utime.tv_sec = utime_ns / 1_000_000_000;
utime.tv_nsec = utime_ns % 1_000_000_000;

if (Mono.Unix.Native.Syscall.utimensat(Mono.Unix.Native.Syscall.AT_FDCWD, path, new[] { atime, utime }, 0) == 0) return;
throw GetLastUnixError(path);
}

[Documentation("utime(path, times=None, *[, ns], dir_fd=None, follow_symlinks=True)")]
public static void utime([NotNone] string path, [ParamDictionary, NotNone] IDictionary<string, object> kwargs, [NotNone] params object[] args) {
var numArgs = args.Length;
Expand Down Expand Up @@ -1979,8 +1878,7 @@ public static int write(CodeContext/*!*/ context, int fd, [NotNone] IBufferProto
public static void kill(CodeContext/*!*/ context, int pid, int sig) {
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) ||
RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) {
if (Mono.Unix.Native.Syscall.kill(pid, Mono.Unix.Native.NativeConvert.ToSignum(sig)) == 0) return;
throw GetLastUnixError();
killUnix(pid, sig);
} else {
if (PythonSignal.NativeSignal.GenerateConsoleCtrlEvent((uint)sig, (uint)pid)) return;

Expand Down Expand Up @@ -2414,20 +2312,19 @@ private static Exception DirectoryExistsError(string? filename) {
return GetOsError(PythonErrno.EEXIST, filename);
}

#if FEATURE_NATIVE

internal static Exception GetLastUnixError(string? filename = null, string? filename2 = null)
=> GetOsError(Mono.Unix.Native.NativeConvert.FromErrno(Mono.Unix.Native.Syscall.GetLastError()), filename, filename2);

#endif

private static Exception GetOsError(int error, string? filename = null, string? filename2 = null)
=> PythonOps.OSError(error, strerror(error), filename, null, filename2);

#if FEATURE_NATIVE || FEATURE_CTYPES

[SupportedOSPlatform("windows")]
internal static Exception GetLastWin32Error(string? filename = null, string? filename2 = null)
=> GetWin32Error(Marshal.GetLastWin32Error(), filename, filename2);

// Gets an error message for a Win32 error code.
internal static string GetMessage(int errorCode) {
[SupportedOSPlatform("windows")]
private static string GetWin32ErrorMessage(int errorCode) {
string msg = new Win32Exception(errorCode).Message;
// error codes: https://docs.microsoft.com/en-us/windows/win32/debug/system-error-codes
if (errorCode is not (< 0 or >= 8200 or 34 or 106 or 317 or 718)) {
Expand All @@ -2440,11 +2337,9 @@ internal static string GetMessage(int errorCode) {
return msg.TrimEnd('\r', '\n', '.');
}

internal static Exception GetLastWin32Error(string? filename = null, string? filename2 = null)
=> GetWin32Error(Marshal.GetLastWin32Error(), filename, filename2);

[SupportedOSPlatform("windows")]
private static Exception GetWin32Error(int error, string? filename = null, string? filename2 = null) {
var msg = GetMessage(error);
var msg = GetWin32ErrorMessage(error);
return PythonOps.OSError(0, msg, filename, error, filename2);
}

Expand Down
Loading