bpo-45621: Small changes to mmap (GH-29247)

* Small tidy-ups / comments
* Use randomized names when testing tagged mmaps to avoid any risk of parallel tests treading on each others' toes

Author: tjguk

Lib/test/test_mmap.py

@@ -7,6 +7,7 @@
 import itertools
 import random
 import socket
+import string
 import sys
 import weakref
 
@@ -15,6 +16,10 @@
 
 PAGESIZE = mmap.PAGESIZE
 
+tagname_prefix = f'python_{os.getpid()}_test_mmap'
+def random_tagname(length=10):
+    suffix = ''.join(random.choices(string.ascii_uppercase, k=length))
+    return f'{tagname_prefix}_{suffix}'
 
 class MmapTests(unittest.TestCase):
 
@@ -610,21 +615,23 @@ def test_tagname(self):
         data1 = b"0123456789"
         data2 = b"abcdefghij"
         assert len(data1) == len(data2)
+        tagname1 = random_tagname()
+        tagname2 = random_tagname()
 
         # Test same tag
-        m1 = mmap.mmap(-1, len(data1), tagname="foo")
+        m1 = mmap.mmap(-1, len(data1), tagname=tagname1)
         m1[:] = data1
-        m2 = mmap.mmap(-1, len(data2), tagname="foo")
+        m2 = mmap.mmap(-1, len(data2), tagname=tagname1)
         m2[:] = data2
         self.assertEqual(m1[:], data2)
         self.assertEqual(m2[:], data2)
         m2.close()
         m1.close()
 
         # Test different tag
-        m1 = mmap.mmap(-1, len(data1), tagname="foo")
+        m1 = mmap.mmap(-1, len(data1), tagname=tagname1)
         m1[:] = data1
-        m2 = mmap.mmap(-1, len(data2), tagname="boo")
+        m2 = mmap.mmap(-1, len(data2), tagname=tagname2)
         m2[:] = data2
         self.assertEqual(m1[:], data1)
         self.assertEqual(m2[:], data2)
@@ -635,17 +642,18 @@ def test_tagname(self):
     @unittest.skipUnless(os.name == 'nt', 'requires Windows')
     def test_sizeof(self):
         m1 = mmap.mmap(-1, 100)
-        tagname = "foo"
+        tagname = random_tagname()
         m2 = mmap.mmap(-1, 100, tagname=tagname)
         self.assertEqual(sys.getsizeof(m2),
                          sys.getsizeof(m1) + len(tagname) + 1)
 
     @unittest.skipUnless(os.name == 'nt', 'requires Windows')
     def test_crasher_on_windows(self):
         # Should not crash (Issue 1733986)
-        m = mmap.mmap(-1, 1000, tagname="foo")
+        tagname = random_tagname()
+        m = mmap.mmap(-1, 1000, tagname=tagname)
         try:
-            mmap.mmap(-1, 5000, tagname="foo")[:] # same tagname, but larger size
+            mmap.mmap(-1, 5000, tagname=tagname)[:] # same tagname, but larger size
         except:
             pass
         m.close()
@@ -857,7 +865,7 @@ def test_resize_succeeds_with_error_for_second_named_mapping(self):
         """
         start_size = 2 * PAGESIZE
         reduced_size = PAGESIZE
-        tagname = "TEST"
+        tagname =  random_tagname()
         data_length = 8
         data = bytes(random.getrandbits(8) for _ in range(data_length))
 

Modules/mmapmodule.c

@@ -32,7 +32,6 @@
 
 #ifdef MS_WINDOWS
 #include <windows.h>
-#include <winternl.h>
 static int
 my_getpagesize(void)
 {
@@ -505,21 +504,6 @@ mmap_resize_method(mmap_object *self,
     }
 
     {
-        /*
-        To resize an mmap on Windows:
-
-        - Close the existing mapping
-        - If the mapping is backed to a named file:
-            unmap the view, clear the data, and resize the file
-            If the file can't be resized (eg because it has other mapped references
-            to it) then let the mapping be recreated at the original size and set
-            an error code so an exception will be raised.
-        - Create a new mapping of the relevant size to the same file
-        - Map a new view of the resized file
-        - If the mapping is backed by the pagefile:
-            copy any previous data into the new mapped area
-            unmap the original view which will release the memory
-        */
 #ifdef MS_WINDOWS
         DWORD error = 0, file_resize_error = 0;
         char* old_data = self->data;
@@ -567,6 +551,11 @@ mmap_resize_method(mmap_object *self,
             self->tagname);
 
         error = GetLastError();
+        /* ERROR_ALREADY_EXISTS implies that between our closing the handle above and
+        calling CreateFileMapping here, someone's created a different mapping with
+        the same name. There's nothing we can usefully do so we invalidate our
+        mapping and error out.
+        */
         if (error == ERROR_ALREADY_EXISTS) {
             CloseHandle(self->map_handle);
             self->map_handle = NULL;