This is the mail archive of the cygwin@sources.redhat.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

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


123456789012345678901234567890123456789012345678901234567890123456789012
34567890
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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]