PATCH: Make libmmalloc work properly with /dev/zero

PATCH: Make libmmalloc work properly with /dev/zero

[Scott Pakin]

I just tried using libmmalloc for the first time, and I was unable to
get it to work when allocating memory from /dev/zero (e.g., by using
"mmalloc_attach (-1, NULL)" to mmap() it).  libmmalloc bus errors when
it tries to write to the allocated region.  The library works fine
when I use an ordinary file, however.

I'm using the libmmalloc.a that comes with gdb-5.1, and I'm running
under Linux 2.4.2.  /dev/zero has 0666 permissions.

I believe the bug is that libmmalloc always mmap()s files with
MAP_SHARED, but it ought to map /dev/zero with MAP_PRIVATE, because
/dev/zero really isn't writable.  Attached below is a patch to
mmap-sup.c that makes mmalloc conditionally use one of MAP_SHARED or
MAP_PRIVATE, based on whether or not the library explicitly opened
/dev/zero.

Of course, if there's some subtle reason the code was written the way
it was -- or if I'm doing something egregiously wrong -- please let me
know.

-- Scott

================== diff -Naur mmap-sup.c.ORIG mmap-sup.c ==================
--- mmap-sup.c.ORIG	Tue Mar  5 13:53:57 2002
+++ mmap-sup.c	Tue Mar  5 14:12:16 2002
@@ -48,6 +48,15 @@
  #define PAGE_ALIGN(addr) (caddr_t) (((long)(addr) + pagesize - 1) & \
  	 
		    ~(pagesize - 1))

+
+/* Return MAP_PRIVATE if MDP represents /dev/zero.  Otherwise, return
+   MAP_SHARED. */
+
+#define MAP_PRIVATE_OR_SHARED(MDP) ((MDP -> flags & MMALLOC_DEVZERO) ? \
+ 
			    MAP_PRIVATE :                      \
+ 
			    MAP_SHARED)
+
+
  /*  Get core for the memory region specified by MDP, using SIZE as the
      amount to either add to or subtract from the existing region.  Works
      like sbrk(), but using mmap(). */
@@ -113,7 +122,7 @@
  	    {
  	      /* Let mmap pick the map start address */
  	      mapto = mmap (0, mapbytes, PROT_READ | PROT_WRITE,
- 
		    MAP_SHARED, mdp -> fd, foffset);
+ 
		    MAP_PRIVATE_OR_SHARED(mdp), mdp -> fd, foffset);
  	      if (mapto != (caddr_t) -1)
  		{
  		  mdp -> base = mdp -> breakval = mapto;
@@ -125,7 +134,8 @@
  	  else
  	    {
  	      mapto = mmap (mdp -> top, mapbytes, PROT_READ | PROT_WRITE,
- 
		    MAP_SHARED | MAP_FIXED, mdp -> fd, foffset);
+ 
		    MAP_PRIVATE_OR_SHARED(mdp) | MAP_FIXED, mdp -> fd,
+ 
		    foffset);
  	      if (mapto == mdp -> top)
  		{
  		  mdp -> top = moveto;
@@ -152,7 +162,7 @@
    /* FIXME:  Quick hack, needs error checking and other attention. */

    base = mmap (mdp -> base, mdp -> top - mdp -> base,
- 
        PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
+ 
        PROT_READ | PROT_WRITE, MAP_PRIVATE_OR_SHARED(mdp) | MAP_FIXED,
  	       mdp -> fd, 0);
    return ((PTR) base);
  }
@@ -166,13 +176,13 @@
    caddr_t base = NULL;

  #ifdef MAP_ANONYMOUS
-  flags = MAP_SHARED | MAP_ANONYMOUS;
+  flags = MAP_PRIVATE | MAP_ANONYMOUS;
    fd = -1;
  #else
  #ifdef MAP_FILE
-  flags = MAP_SHARED | MAP_FILE;
+  flags = MAP_PRIVATE | MAP_FILE;
  #else
-  flags = MAP_SHARED;
+  flags = MAP_PRIVATE;
  #endif
    fd = open ("/dev/zero", O_RDWR);
    if (fd != -1)

Re: PATCH: Make libmmalloc work properly with /dev/zero

[Andrew Cagney]

> I just tried using libmmalloc for the first time, and I was unable to
> get it to work when allocating memory from /dev/zero (e.g., by using
> "mmalloc_attach (-1, NULL)" to mmap() it).  libmmalloc bus errors when
> it tries to write to the allocated region.  The library works fine
> when I use an ordinary file, however.
> 
> I'm using the libmmalloc.a that comes with gdb-5.1, and I'm running
> under Linux 2.4.2.  /dev/zero has 0666 permissions.
> 
> I believe the bug is that libmmalloc always mmap()s files with
> MAP_SHARED, but it ought to map /dev/zero with MAP_PRIVATE, because
> /dev/zero really isn't writable.  Attached below is a patch to
> mmap-sup.c that makes mmalloc conditionally use one of MAP_SHARED or
> MAP_PRIVATE, based on whether or not the library explicitly opened
> /dev/zero.
> 
> Of course, if there's some subtle reason the code was written the way
> it was -- or if I'm doing something egregiously wrong -- please let me
> know.

Fred's ok with this patch.  However, I can't get it to apply - something 
ate something while it was in transit?  Perhaphs post it as a mime 
attachment?

I'll also need a ChangeLog entry.

nice catch,
Andrew

Re: PATCH: Make libmmalloc work properly with /dev/zero

[Scott Pakin]

[-- Attachment #1: Type: text/plain, Size: 515 bytes --]

On 23MAR2002, Andrew Cagney wrote:

> Fred's ok with this patch.  However, I can't get it to apply - something 
> ate something while it was in transit?  Perhaphs post it as a mime 
> attachment?

Attached.  Let me know if that still doesn't work.

> I'll also need a ChangeLog entry.

2002-03-05  Scott Pakin <pakin@uiuc.edu>

         * Modified calls to mmap() to make mmap() use MAP_PRIVATE when
           MDP represents /dev/zero.  In all other cases, mmap() continues
           to use MAP_SHARED.

-- Scott

[-- Attachment #2: mmap-sup-map_private.patch --]
[-- Type: text/plain, Size: 1943 bytes --]

--- mmap-sup.c.ORIG	Tue Mar  5 13:53:57 2002
+++ mmap-sup.c	Tue Mar  5 14:12:16 2002
@@ -48,6 +48,15 @@
 #define PAGE_ALIGN(addr) (caddr_t) (((long)(addr) + pagesize - 1) & \
 				    ~(pagesize - 1))
 
+
+/* Return MAP_PRIVATE if MDP represents /dev/zero.  Otherwise, return
+   MAP_SHARED. */
+
+#define MAP_PRIVATE_OR_SHARED(MDP) ((MDP -> flags & MMALLOC_DEVZERO) ? \
+				    MAP_PRIVATE :                      \
+				    MAP_SHARED)
+
+
 /*  Get core for the memory region specified by MDP, using SIZE as the
     amount to either add to or subtract from the existing region.  Works
     like sbrk(), but using mmap(). */
@@ -113,7 +122,7 @@
 	    {
 	      /* Let mmap pick the map start address */
 	      mapto = mmap (0, mapbytes, PROT_READ | PROT_WRITE,
-			    MAP_SHARED, mdp -> fd, foffset);
+			    MAP_PRIVATE_OR_SHARED(mdp), mdp -> fd, foffset);
 	      if (mapto != (caddr_t) -1)
 		{
 		  mdp -> base = mdp -> breakval = mapto;
@@ -125,7 +134,8 @@
 	  else
 	    {
 	      mapto = mmap (mdp -> top, mapbytes, PROT_READ | PROT_WRITE,
-			    MAP_SHARED | MAP_FIXED, mdp -> fd, foffset);
+			    MAP_PRIVATE_OR_SHARED(mdp) | MAP_FIXED, mdp -> fd,
+			    foffset);
 	      if (mapto == mdp -> top)
 		{
 		  mdp -> top = moveto;
@@ -152,7 +162,7 @@
   /* FIXME:  Quick hack, needs error checking and other attention. */
 
   base = mmap (mdp -> base, mdp -> top - mdp -> base,
-	       PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
+	       PROT_READ | PROT_WRITE, MAP_PRIVATE_OR_SHARED(mdp) | MAP_FIXED,
 	       mdp -> fd, 0);
   return ((PTR) base);
 }
@@ -166,13 +176,13 @@
   caddr_t base = NULL;
 
 #ifdef MAP_ANONYMOUS
-  flags = MAP_SHARED | MAP_ANONYMOUS;
+  flags = MAP_PRIVATE | MAP_ANONYMOUS;
   fd = -1;
 #else
 #ifdef MAP_FILE
-  flags = MAP_SHARED | MAP_FILE;
+  flags = MAP_PRIVATE | MAP_FILE;
 #else
-  flags = MAP_SHARED;
+  flags = MAP_PRIVATE;
 #endif
   fd = open ("/dev/zero", O_RDWR);
   if (fd != -1)

Re: PATCH: Make libmmalloc work properly with /dev/zero

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 189 bytes --]

FYI,

I've checked in the attached.  I tweaked the code and ChangeLog to 
comply with GNU conventions (and resisted the temptation to change the 
macro into a function :-).

thanks!
Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 2371 bytes --]

2002-04-07  Andrew Cagney  <ac131313@redhat.com>

	From 2002-03-05 Scott Pakin <pakin@uiuc.edu>
	* mmap-sup.c (MAP_PRIVATE_OR_SHARED): Define.
	(__mmalloc_mmap_morecore): Use.
	(__mmalloc_remap_core): Use.
	
Index: mmap-sup.c
===================================================================
RCS file: /cvs/src/src/mmalloc/mmap-sup.c,v
retrieving revision 1.3
diff -u -r1.3 mmap-sup.c
--- mmap-sup.c	19 Feb 2000 00:44:46 -0000	1.3
+++ mmap-sup.c	7 Apr 2002 17:53:23 -0000
@@ -48,6 +48,15 @@
 #define PAGE_ALIGN(addr) (caddr_t) (((long)(addr) + pagesize - 1) & \
 				    ~(pagesize - 1))
 
+
+/* Return MAP_PRIVATE if MDP represents /dev/zero.  Otherwise, return
+   MAP_SHARED.  */
+
+#define MAP_PRIVATE_OR_SHARED(MDP) ((MDP -> flags & MMALLOC_DEVZERO) \
+                                    ? MAP_PRIVATE \
+                                    : MAP_SHARED)
+
+
 /*  Get core for the memory region specified by MDP, using SIZE as the
     amount to either add to or subtract from the existing region.  Works
     like sbrk(), but using mmap(). */
@@ -113,7 +122,7 @@
 	    {
 	      /* Let mmap pick the map start address */
 	      mapto = mmap (0, mapbytes, PROT_READ | PROT_WRITE,
-			    MAP_SHARED, mdp -> fd, foffset);
+			    MAP_PRIVATE_OR_SHARED (mdp), mdp -> fd, foffset);
 	      if (mapto != (caddr_t) -1)
 		{
 		  mdp -> base = mdp -> breakval = mapto;
@@ -125,7 +134,8 @@
 	  else
 	    {
 	      mapto = mmap (mdp -> top, mapbytes, PROT_READ | PROT_WRITE,
-			    MAP_SHARED | MAP_FIXED, mdp -> fd, foffset);
+			    MAP_PRIVATE_OR_SHARED (mdp) | MAP_FIXED, mdp -> fd,
+			    foffset);
 	      if (mapto == mdp -> top)
 		{
 		  mdp -> top = moveto;
@@ -152,7 +162,7 @@
   /* FIXME:  Quick hack, needs error checking and other attention. */
 
   base = mmap (mdp -> base, mdp -> top - mdp -> base,
-	       PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED,
+	       PROT_READ | PROT_WRITE, MAP_PRIVATE_OR_SHARED (mdp) | MAP_FIXED,
 	       mdp -> fd, 0);
   return ((PTR) base);
 }
@@ -166,13 +176,13 @@
   caddr_t base = NULL;
 
 #ifdef MAP_ANONYMOUS
-  flags = MAP_SHARED | MAP_ANONYMOUS;
+  flags = MAP_PRIVATE | MAP_ANONYMOUS;
   fd = -1;
 #else
 #ifdef MAP_FILE
-  flags = MAP_SHARED | MAP_FILE;
+  flags = MAP_PRIVATE | MAP_FILE;
 #else
-  flags = MAP_SHARED;
+  flags = MAP_PRIVATE;
 #endif
   fd = open ("/dev/zero", O_RDWR);
   if (fd != -1)