All commit authors signed the Contributor License Agreement.

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

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

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

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

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

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

One other possibility may be to protectPyBytes_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, theMapViewOfFile family are the only functions I can find which reference theEXCEPTION_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 whodoes 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 ifsafe_memcpy(...) orsafe_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 POSIXSIGBUS andSIGSEGV 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 themmap type, it should be addressed on POSIX as well as Windows.

I usedLsaNtStatusToWinError to convert the error code whichshould do they same asRtlNtStatusToDosError but easier to load. I created a function callsafe_memcpy which returns0 if it succeeds and-1 otherwise and restructuredmmap_read_byte_method andmmap_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 likesafe_memchr formmap_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 (likemmap_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 tqdmfromcwinsdk.um.winternlimportRtlNtStatusToDosErrorfromcwinsdk.um.ntsecapiimportLsaNtStatusToWinErrorfromtqdmimporttrangeforiintrange(-(2**31),2**31):a=RtlNtStatusToDosError(i)b=LsaNtStatusToWinError(i)ifa!=b:raiseAssertionError(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 likeRtlNtStatusToDosError 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 likeRtlNtStatusToDosError though.

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

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

!buildbot .windows.

🤖 New build scheduled with the buildbot fleet by@zooba for commit9737f22 🤖

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 ofrestrict based on the compiler version (or the level of C99 support) -restrict is notone 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 removerestrict. I only used it because it was part of the signature formemcpy here for C99https://en.cppreference.com/w/c/string/byte/memcpy

We can also simply removerestrict.

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 the3.13 branch.