[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: Review Request: Fix KPixmapCache crash (probably because of
From:       Michael Pyne <mpyne () purinchu ! net>
Date:       2009-05-29 1:04:19
Message-ID: 200905282104.23678.mpyne () purinchu ! net
[Download RAW message or body]

[Attachment #2 (multipart/mixed)]


On Monday 25 May 2009 03:55:37 Johannes Sixt wrote:
> No, don't ship it. The new code is just as wrong as the old code was.
>
> The new code uses the idiom
>     *(some_type*)foo
> where foo is of some_other_type. This breaks aliasing rules.

I have a patch (attached) for the issue, which eliminates the union entirely.  
The portions of code needing a char* pointer to the beginning of mmap'ed 
memory get one using a reinterpret_cast, and the DataHeader alias was unneeded 
anyways from what I can tell.

I plan to commit to 4.3 and 4.2 branches tonight after I do some testing here.

Regards,
 - Michael Pyne

["kpixmapcache-alignment.patch" (text/x-patch)]

Index: kdeui/util/kpixmapcache.cpp
===================================================================
--- kdeui/util/kpixmapcache.cpp	(revision 973868)
+++ kdeui/util/kpixmapcache.cpp	(working copy)
@@ -289,15 +289,11 @@
     // Holds info about mmapped file
     struct MmapInfo
     {
-        MmapInfo()  { file = 0; memory = 0; }
+        MmapInfo()  { file = 0; indexHeader = 0; }
         QFile* file;  // If this is not null, then the file is mmapped
 
-        // Convenience aliases.  This probably breaks some C++ aliasing rule or \
                something. :-/
-        union {
-            char* memory;
-            KPixmapCacheIndexHeader *indexHeader;
-            KPixmapCacheDataHeader *dataHeader;
-        };
+        // This points to the mmap'ed file area.
+        KPixmapCacheIndexHeader *indexHeader;
 
         quint32 size;  // Number of currently used bytes
         quint32 available;  // Number of available bytes (including those reserved \
for mmap) @@ -474,9 +470,9 @@
         info->file = 0;
         return false;
     }
-    info->memory = reinterpret_cast<char*>(indexMem);
+    info->indexHeader = reinterpret_cast<KPixmapCacheIndexHeader *>(indexMem);
 #ifdef HAVE_MADVISE
-    madvise(info->memory, info->size, MADV_WILLNEED);
+    madvise(indexMem, info->size, MADV_WILLNEED);
 #endif
 
     info->file->close();
@@ -496,8 +492,8 @@
 void KPixmapCache::Private::unmmapFile(MmapInfo* info)
 {
     if (info->file) {
-        info->file->unmap(reinterpret_cast<uchar*>(info->memory));
-        info->memory = 0;
+        info->file->unmap(reinterpret_cast<uchar*>(info->indexHeader));
+        info->indexHeader = 0;
         info->available = 0;
         info->size = 0;
 
@@ -523,7 +519,9 @@
         fi.refresh();
         if(fi.exists() && fi.size() == mIndexMmapInfo.available) {
             // Create the device
-            device = new KPCMemoryDevice(mIndexMmapInfo.memory, \
&mIndexMmapInfo.size, mIndexMmapInfo.available); +            device = new \
KPCMemoryDevice( +                             \
reinterpret_cast<char*>(mIndexMmapInfo.indexHeader), +                             \
&mIndexMmapInfo.size, mIndexMmapInfo.available);  }
 
         // Is it possible to have a valid cache with no file?  If not it would be \
easier @@ -592,7 +590,9 @@
         fi.refresh();
         if (fi.exists() && fi.size() == mDataMmapInfo.available) {
             // Create the device
-            return new KPCMemoryDevice(mDataMmapInfo.memory, &mDataMmapInfo.size, \
mDataMmapInfo.available); +            return new KPCMemoryDevice(
+                           reinterpret_cast<char*>(mDataMmapInfo.indexHeader),
+                           &mDataMmapInfo.size, mDataMmapInfo.available);
         }
         else
             return 0;


["signature.asc" (application/pgp-signature)]

[prev in list] [next in list] [prev in thread] [next in thread]