All commit authors signed the Contributor License Agreement.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

One other possibility may be to protect PyBytes_FromString internally, which could cut off an entire class of crashes instead of just one. Thoughts?

That's a possibility too. However I don't know if there's a big performance impact if the exception handling is used there. PyBytes_FromString seems to be a quite commonly used function. On the other hand, the MapViewOfFile family are the only functions I can find which reference the EXCEPTION_IN_PAGE_ERROR exception.

I don't know if there's a big performance impact if the exception handling is used there.

Native exceptions are zero cost until an exception is actually thrown, so I wouldn't worry about that. The compiler generates a static table that the OS refers to in order to figure out which handler to invoke.

That said, a number of the other functions you mentioned won't go through that path, so there's probably going to be a need to add it directly into mmap, particularly for writing.

Maybe a helper function to do a protected memcpy is the way to go? That would let us entirely scope the exception handling and make sure we aren't mismanaging any state inside of it (which is the usual issue). It also means we can have a plain implementation on non-Windows (for now) and keep the code that uses it looking fairly consistent.

Maybe a helper function to do a protected memcpy is the way to go?

I think that's probably a good idea. However there are still some rogue pointer derefs (like the one-character optimization mentioned above) which can still throw.

EDIT: This is out-of-scope for this PR. But Linux probably has similar issues. I couldn't find good docs regarding the situtation (I am more familiar with Windows), but Linux will probably send some signal like SIGBUS/SIGSEV in this scenario. Are you supposed to handle them when mmapping?

Linux probably has similar issues. I couldn't find good docs regarding the situtation (I am more familiar with Windows), but Linux will probably send some signal like SIGBUS/SIGSEV in this scenario

I'm in the same position as you, and agree that it's out of scope for this PR. But if we set things up so that it's easy to multiplex per-platform, we'll at least set up someone who does know to be able to do it.

What that means to me is that we want to contain __try/__except to helper functions that don't interact with Python objects at all (you can assume that the caller has "pinned" any memory that gets passed in) and have an error result that basically means "invalid pointer". So if safe_memcpy(...) or safe_read_char(char *dest, const char *src) return < 0 (typical for our APIs), then we can raise an error on all platforms, and eventually maybe they get implementations that are more complex than just *dest = *src; return 0;

Linux will probably send some signal like SIGBUS/SIGSEV in this scenario. Are you supposed to handle them when mmapping?

Yes, on POSIX SIGBUS and SIGSEGV are raised synchronously on the executing thread. I mentioned this on the linked issue. I think if this crash scenario is to be handled by the mmap type, it should be addressed on POSIX as well as Windows.

I used LsaNtStatusToWinError to convert the error code which should do they same as RtlNtStatusToDosError but easier to load. I created a function call safe_memcpy which returns 0 if it succeeds and -1 otherwise and restructured mmap_read_byte_method and mmap_read_method to use it.

It will be very difficult to handle all the functions in this way though. For example there would need to be additional functions like safe_memchr for mmap_read_line_method and for other functions I need to go even deeper to see if they need any cleanup or can simply be wrapped (like mmap_find_method->mmap_gfind->_PyBytes_Find->stringlib_find->???)

By the way regarding the ntstatus to win32 error conversion
I ran

# pip install ctypes-windows-sdk tqdm
from cwinsdk.um.winternl import RtlNtStatusToDosError
from cwinsdk.um.ntsecapi import LsaNtStatusToWinError

from tqdm import trange

for i in trange(-(2**31), 2**31):
    a = RtlNtStatusToDosError(i)
    b = LsaNtStatusToWinError(i)
    if a != b:
        raise AssertionError(f"NTSTATUS={i}, RtlNtStatusToDosError={a}, LsaNtStatusToWinError={b}")

and it shows that the output of both functions is the same for the full range of ints. LsaNtStatusToWinError doesn't have to be manually loaded like RtlNtStatusToDosError though.

it shows that the output of both functions is the same for the full range of ints. LsaNtStatusToWinError doesn't have to be manually loaded like RtlNtStatusToDosError though.

I forgot about that one. It's not exactly surprising that it behaves the same. 😉

0:005> u advapi32!LsaNtStatusToWinError l1
ADVAPI32!LsaNtStatusToWinError:
00007ff9`32869740 48ff2569430700  jmp     qword ptr [ADVAPI32!_imp_RtlNtStatusToDosError (00007ff9`328ddab0)]
0:005> dq 00007ff9`328ddab0 l1
00007ff9`328ddab0  00007ff9`32e23140
0:005> ln 00007ff9`32e23140
(00007ff9`32e23140)   ntdll!RtlNtStatusToDosError   |  (00007ff9`32e232e0)   ntdll!RtlSetLastWin32Error
Exact matches:
    ntdll!RtlNtStatusToDosError (void)

!buildbot .windows.

🤖 New build scheduled with the buildbot fleet by @zooba for commit 9737f22 🤖

The command will test the builders whose names match following regular expression: .*windows.*

The builders matched are:

• ARM64 Windows Non-Debug PR
• AMD64 Windows10 PR
• AMD64 Windows Server 2022 NoGIL PR
• AMD64 Windows11 Non-Debug PR
• AMD64 Windows11 Bigmem PR
• AMD64 Windows11 Refleaks PR
• ARM64 Windows PR

I'm torn between just merging (to make beta 1) and being more thorough, but I think we should be more thorough. This can easily go in for beta 2, so the only urgency is to make sure we get good test coverage.

Looks like we may need to use or switch to __restrict instead of restrict based on the compiler version (or the level of C99 support) - restrict is not one of our listed requirements, and it seems easy enough to avoid:

#if defined(_MSC_VER) && (!defined(__STDC_VERSION__) || (__STDC_VERSION__+0) < 201112L)
#define restrict __restrict
#endif

(Possibly that condition has to be broken up, but I think it ought to work.)

We can also simply remove restrict. I only used it because it was part of the signature for memcpy here for C99 https://en.cppreference.com/w/c/string/byte/memcpy

We can also simply remove restrict.

Works for me.

We'll get this in for beta 2 at this stage.

@zooba @eryksun Thank you so much for all your help! This is my first PR for CPython and I had a very good experience.

Congratulations!

Thanks @Dobatymo for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

GH-118887 is a backport of this pull request to the 3.13 branch.