tags 250583 patch
thanks

On Tuesday 27 September 2005 10:55 am, Michael Vogt wrote:
> I'm currently clueless about this bug. Each mmap is unmaped (I verfied
> with both strace and some debug code mmap.cc), the file handles are
> closed. But apparently it's not freed.

  Here's an interesting point.  Watch what happens from one run to another:

  Run 1:
/proc/8089/maps:b3ccc000-b4487000 r--s 00000000 fe:00 1376292    
/var/cache/apt/pkgcache.bin
/proc/8089/maps:b4487000-b4488000 rw-s 00c00000 fe:00 1376292    
/var/cache/apt/pkgcache.bin
/proc/8089/maps:b4488000-b4489000 rw-s 00c00000 fe:00 1376291    
/var/cache/apt/pkgcache.bin (deleted)
/proc/8089/maps:b4489000-b448a000 rw-s 00c00000 fe:00 1376287    
/var/cache/apt/pkgcache.bin (deleted)
/proc/8089/maps:b448a000-b448b000 rw-s 00c00000 fe:00 1376285    
/var/cache/apt/pkgcache.bin (deleted)

  Run 2:
/proc/8089/maps:b3ccb000-b4486000 r--s 00000000 fe:00 1376293    
/var/cache/apt/pkgcache.bin
/proc/8089/maps:b4486000-b4487000 rw-s 00c00000 fe:00 1376293    
/var/cache/apt/pkgcache.bin
/proc/8089/maps:b4487000-b4488000 rw-s 00c00000 fe:00 1376292    
/var/cache/apt/pkgcache.bin (deleted)
/proc/8089/maps:b4488000-b4489000 rw-s 00c00000 fe:00 1376291    
/var/cache/apt/pkgcache.bin (deleted)
/proc/8089/maps:b4489000-b448a000 rw-s 00c00000 fe:00 1376287    
/var/cache/apt/pkgcache.bin (deleted)
/proc/8089/maps:b448a000-b448b000 rw-s 00c00000 fe:00 1376285    
/var/cache/apt/pkgcache.bin (deleted)

  The (deleted) indicates that the backing file was unlinked.  What I find
interesting here is that the remaining mappings are all exactly one memory
page large, and each of them is the last memory page of some previous
mapping.  However, it is NOT the last page of the mapping that's created
by the code that creates the top mapping in the list -- otherwise it would
have the same flags.  Following this logic, I was able to find these two
events in strace:

mmap2(NULL, 12582913, PROT_READ|PROT_WRITE, MAP_SHARED, 4, 0) = 0xb3886000
munmap(0xb3886000, 12582912)            = 0

  12582912 is an exact multiple of the page size (4096).  Thus, mapping
12582913 bytes and then unmapping 12582912 will leak a single page.

  Why is this happening?  Look at the DynamicMMap constructor and
destructor:

DynamicMMap::DynamicMMap(FileFd &F,unsigned long Flags,unsigned long WorkSpace) 
: 
             MMap(F,Flags | NoImmMap), Fd(&F), WorkSpace(WorkSpace)
{
   if (_error->PendingError() == true)
      return;
   
   unsigned long EndOfFile = Fd->Size();
   if (EndOfFile > WorkSpace)
      WorkSpace = EndOfFile;
   else
   {
      Fd->Seek(WorkSpace);
      char C = 0;
      Fd->Write(&C,sizeof(C));
   }
   
   Map(F);
   iSize = EndOfFile;
}

DynamicMMap::~DynamicMMap()
{
   if (Fd == 0)
   {
      delete [] (unsigned char *)Base;
      return;
   }
   
   unsigned long EndOfFile = iSize;
   iSize = WorkSpace;
   Close(false);
   ftruncate(Fd->Fd(),EndOfFile);
}  

  What happens here is: WorkSpace is the size of the cache to create.  The
constructor seeks to location Workspace in the file and writes a single byte.
This causes the file to become **WorkSpace+1** bytes long.  Map() then sets
iSize to the exact size of the file and maps that many bytes.  In the
destructor, iSize is set to WorkSpace, and so WorkSpace bytes are unmapped
from the cache -- hence the discrepency.

  The fix is obvious: if WorkSpace is greater than zero,
seek to one less than WorkSpace bytes in the constructor (if WorkSpace
is zero, do nothing).  The attached patch does just this, and eliminates
the bug on unstable for me.

  Daniel

-- 
/------------------- Daniel Burrows <[EMAIL PROTECTED]> ------------------\
|         "This is too absurd!  The world can't end this stupidly!"         |
|         "Oh, sure it can.  Have some faith."                              |
|           -- Fluble                                                       |
\------------ Got APT? -- Debian GNU/Linux http://www.debian.org -----------/
--- mmap.cc.old	2005-08-01 09:05:41.000000000 -0700
+++ mmap.cc	2005-09-27 18:41:30.622947971 -0700
@@ -155,9 +155,9 @@
    unsigned long EndOfFile = Fd->Size();
    if (EndOfFile > WorkSpace)
       WorkSpace = EndOfFile;
-   else
+   else if(WorkSpace > 0)
    {
-      Fd->Seek(WorkSpace);
+      Fd->Seek(WorkSpace - 1);
       char C = 0;
       Fd->Write(&C,sizeof(C));
    }

Attachment: pgpE7n6KsCSev.pgp
Description: PGP signature

Reply via email to