[PATCH] Problems with Cygwin mmap/munmap (mmap.cc) [correctly formatted]

Kelley Cook Kelley.Cook@home.com
Tue Nov 28 11:30:00 GMT 2000


Cygwin developers:

While I was researching why the current snapshot of GCC is not bootstrapping, 
Zack Weinberg discovered that cygwin implementation of mmap/munmap is not
quite correctly implemented.

Cygwin's munmap does not use the length parameter at all.  Under most *nixes 
you can 

loc = mmap (NULL, 1048576, PROT_READ | PROT_WRITE, 
            MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
munmap (loc + 786432, 262144)
munmap (loc, 524288)

and still have a valid 256K map @ loc+524288.

This fails under cygwin since munmap is a thunk into the Win32's API 
UnmapViewOfFile which does not take a length parameter and therefore always 
frees the amount allocated on the call to MapViewOfFile/MapViewOfFileEx.  
So under Cygwin, the first munmap call would fail and the second one 
frees the entire range.  The first causes a memory leak and the second
will likely later lead to a segfault.

I fixed this in the patch below.  Essentially it will internally split 
the mmaped region into two mapped regions if the requested unmapped 
region is in the middle of a mapped region.  It only really unmaps 
the memory if there are no more splits left.

Unfortunately I do not know a way of testing the change to 
fixup_mmaps_after_fork (), but it should be fairly straightforeward.

GCC 2.97 20001120 successfully bootstraps after this change.

straces now look good when compiling c-decl.c, which is among those files 
that previously crashed when compiling GCC 2.97 under Cygwin.

FWIW, it doesn't attempt to emulate another *nix mmap/munmap possibility,
which is using one munmap with a double length to unmap two consecutive 
mmapped regions.  I personally don't think this is a good idea, so I 
just ignored that case.  The munmap call properly fails with EINVAL if it
is attempted which at least will let the programmer know that it is 
unsupported.

Tue Nov 28 18:21:48 2000 Kelley Cook <kelley.cook@home.com>

    * mmap.cc (mmap_record): Add in original_size_ and _original_address.
              (mmap): Likewise.
              (fixup_mmaps_after_fork): Likewise. Only remap unique addresses.
              (munmap): Handle split munmaps. Only actually unmap unique addresses.

--- cygwin/mmap.cc.orig	Mon Nov 27 14:14:17 2000
+++ cygwin/mmap.cc	Tue Nov 28 13:21:48 2000
@@ -33,13 +33,17 @@ class mmap_record
     HANDLE mapping_handle_;
     DWORD access_mode_;
     DWORD offset_;
+    DWORD original_size_;
     DWORD size_to_map_;
-    void *base_address_;
+    char *original_address_;
+    char *base_address_;
 
   public:
-    mmap_record (HANDLE h, DWORD ac, DWORD o, DWORD s, void *b) :
+    mmap_record (HANDLE h, DWORD ac, DWORD o, DWORD os, DWORD s, char *ob, char *b) :
        mapping_handle_ (h), access_mode_ (ac), offset_ (o),
-       size_to_map_ (s), base_address_ (b) { ; }
+       original_size_ (os), size_to_map_ (s),
+       original_address_ (ob), base_address_ (b) { ; }
+
 
     /* Default Copy constructor/operator=/destructor are ok */
 
@@ -47,8 +51,10 @@ class mmap_record
     HANDLE get_handle () const { return mapping_handle_; }
     DWORD get_access () const { return access_mode_; }
     DWORD get_offset () const { return offset_; }
+    DWORD get_original_size () const { return original_size_; }
     DWORD get_size () const { return size_to_map_; }
-    void *get_address () const { return base_address_; }
+    char *get_original_address () const { return original_address_; }
+    char *get_address () const { return base_address_; }
 };
 
 class list {
@@ -231,7 +237,7 @@ mmap (caddr_t addr, size_t len, int prot
   /* Now we should have a successfully mmaped area.
      Need to save it so forked children can reproduce it.
   */
-  mmap_record mmap_rec (h, access, off, len, base);
+  mmap_record mmap_rec (h, access, off, len, len, base, base);
 
   /* Get list of mmapped areas for this fd, create a new one if
      one does not exist yet.
@@ -292,7 +298,16 @@ munmap (caddr_t addr, size_t len)
 	  for (li = 0; li < l->nrecs; ++li)
 	    {
 	      mmap_record rec = l->recs[li];
-	      if (rec.get_address () == addr)
+
+	      HANDLE r_handle = rec.get_handle ();
+	      DWORD r_access = rec.get_access ();
+	      off_t r_offset = rec.get_offset ();
+	      size_t r_size = rec.get_size ();
+	      size_t r_osize = rec.get_original_size ();
+	      caddr_t r_oaddress = rec.get_original_address ();
+	      caddr_t r_address = rec.get_address ();
+
+	      if (r_address <= addr && r_address + r_size >= addr + len)
 		{
                   int fd = l->fd;
                   fhandler_disk_file fh_paging_file (NULL);
@@ -305,9 +320,55 @@ munmap (caddr_t addr, size_t len)
                     }
                   else
                     fh = fdtab[fd];
-                  fh->munmap (rec.get_handle (), addr, len);
+		  if (r_size == len)
+		    /* Normal munmap where length is same as mmap request. */
+		    {
+		      int lj, count = 0;
+
+		      syscall_printf ("Original Address: %x", r_oaddress);
+		      for (lj = 0; lj < l->nrecs; ++lj)
+			{
+			  mmap_record tmprec = l->recs[lj];
+		          syscall_printf ("Checking %d: %x", lj, 
+                                          tmprec.get_original_address () );
+			  if (tmprec.get_original_address () == r_oaddress)
+			    count++;
+			}
+		      syscall_printf ("number of matches: %d", count);
+		      /* Only call underlying unmap if the original map request
+			 is no longer used by any potential splits. */
+		      if (count == 1)
+			fh->munmap (r_handle, r_oaddress, r_osize);
+		    }
+		  else
+		    /* For those cases where large mmap is munmapped piecemeal. 
+		       We need to create new mmapped regions of the parts that 
+		       will be leftover. */
+		    {
+		      syscall_printf ("split from: %x %d", r_address, r_size);
+		      if (r_address != addr)
+			/* Create new mmapped region before requested munmapped. */
+			{
+			  mmap_record split (r_handle, r_access, r_offset, r_osize,
+					     addr - r_address,
+					     r_oaddress, r_address);
+			  syscall_printf ("split to: %x %d", r_address, 
+                                          addr - r_address);
+			  l->add_record (split);
+			}
+		      if (r_address + r_size != addr + len)
+			/* Create new mmapped region after requested munmapped. */
+			{
+			  mmap_record split (r_handle, r_access, r_offset, r_osize,
+					     r_size - len + r_address - addr,
+					     r_oaddress, addr + len);
+			  syscall_printf ("split to: %x %d", addr+len, 
+                                          r_size - len + r_address -addr);
+			  l->add_record (split);
+			}
+		    }
 
-		  /* Delete the entry. */
+		  /* Delete the original list entry. */
 		  l->erase (li);
 		  syscall_printf ("0 = munmap(): %x", addr);
 		  ReleaseResourceLock(LOCK_MMAP_LIST,WRITE_LOCK|READ_LOCK," munmap");
@@ -578,19 +639,31 @@ fixup_mmaps_after_fork ()
 
 	      debug_printf ("h %x, access %x, offset %d, size %d, address %p",
 		  rec.get_handle (), rec.get_access (), rec.get_offset (),
-		  rec.get_size (), rec.get_address ());
+		  rec.get_original_size (), rec.get_original_address ());
 
-	      /* Now re-create the MapViewOfFileEx call */
-	      void *base = MapViewOfFileEx (rec.get_handle (),
-					    rec.get_access (), 0,
-					    rec.get_offset (),
-					    rec.get_size (),
-					    rec.get_address ());
-	      if (base != rec.get_address ())
+	      int lj = 0, flag = 0;
+
+	      while (lj < li)
+		{
+		  mmap_record tmprec = l->recs[lj];
+		  if (tmprec.get_original_address () == rec.get_original_address ())
+		    flag = 1;
+		  lj++;
+		}
+	      if (flag == 0)
 		{
-		  system_printf ("base address %p fails to match requested address %p",
-				 rec.get_address ());
-		  return -1;
+		/* Now re-create the MapViewOfFileEx call */
+		void *base = MapViewOfFileEx (rec.get_handle (),
+					      rec.get_access (), 0,
+					      rec.get_offset (),
+					      rec.get_original_size (),
+					      rec.get_original_address ());
+		if (base != rec.get_address ())
+		  {
+		    system_printf ("base address %p fails to match requested address %p",
+		  		    base, rec.get_original_address ());
+		    return -1;
+		  }
 		}
 	    }
 	}








--
Want to unsubscribe from this list?
Send a message to cygwin-unsubscribe@sourceware.cygnus.com



More information about the Cygwin mailing list