[PATCH] Enhancements to VCL metafile handling

Hi,

I have submitted a patch for review:

    https://gerrit.libreoffice.org/3839

To pull it, you can do:

    git pull ssh://gerrit.libreoffice.org:29418/core refs/changes/39/3839/1

Enhancements to VCL metafile handling

* Remove EMFP_DEBUG with OSL_DEBUG conditional defines and SAL_INFO
* While we are about it, to help with troubleshooting metafile
  issues, add another vcl.emf area to log-areas.dox
* Improve error handling when processing an EMF header:
    + replace variable name nsal_uInt32 (!!) with sane variables
      to make the code more readable
    + check to ensure that type field is 0x1, which is all it can be
      for metafiles
    + check that signature field is set to ASCII-encoded value "FME"
    + loose check of version field to see if it is 0x00010000
    + warn if record count is zero - that really shouldn't be possible
    + check bytes field in header to make sure it correlates to the
      actual size of the metafile
* Quite a few more comments in the code to clarify the intended
  structure of a metafile, per [MS-EMF] documentation

Change-Id: Id4ed486b2dd0c6e7bdee67cb344aaaf8e8d98f84
---
M include/sal/log-areas.dox
M vcl/source/filter/wmf/enhwmf.cxx
2 files changed, 78 insertions(+), 32 deletions(-)



diff --git a/include/sal/log-areas.dox b/include/sal/log-areas.dox
index 693450b..cf1f1cc 100644
--- a/include/sal/log-areas.dox
+++ b/include/sal/log-areas.dox
@@ -260,6 +260,7 @@
 @li @c vcl.atsui - ATSUI (obsolete) -using code for Mac OS X
 @li @c vcl.control
 @li @c vcl.coretext - CoreText-using code for Mac OS X and iOS
+@li @c vcl.emf - EMF/EMF+ processing
 @li @c vcl.fonts - font-specific code
 @li @c vcl.gdi - the GDI part of VCL, devices, bitmaps, etc.
 @li @c vcl.gtk - Gtk+ 2/3 plugin
diff --git a/vcl/source/filter/wmf/enhwmf.cxx b/vcl/source/filter/wmf/enhwmf.cxx
index cd71a6b..298cd1e 100644
--- a/vcl/source/filter/wmf/enhwmf.cxx
+++ b/vcl/source/filter/wmf/enhwmf.cxx
@@ -151,12 +151,6 @@
 #define EMR_SETLINKEDUFIS              119
 #define EMR_SETTEXTJUSTIFICATION       120
 
-#if OSL_DEBUG_LEVEL > 1
-#define EMFP_DEBUG(x) x
-#else
-#define EMFP_DEBUG(x)
-#endif
-
 
 #ifdef OSL_BIGENDIAN
 // currently unused
@@ -226,24 +220,27 @@
     return bOk;
 }
 
-EMFP_DEBUG(void dumpWords( SvStream& s, int i )
+#if OSL_DEBUG_LEVEL > 1
+void dumpWords( SvStream& s, int i )
 {
     sal_uInt32 pos = s.Tell();
     sal_Int16 data;
     for( ; i > 0; i -- ) {
         s >> data;
-        EMFP_DEBUG(printf ("\t\t\tdata: %04hx\n", data));
+        SAL_INFO("vcl.emf", "\t\t\tdata: " << std::hex << data);
     }
     s.Seek (pos);
-});
+};
+#endif
 
 void EnhWMFReader::ReadEMFPlusComment(sal_uInt32 length, sal_Bool& bHaveDC)
 {
     if (!bEMFPlus) {
         pOut->PassEMFPlusHeaderInfo();
 
+#if OSL_DEBUG_LEVEL > 1
         // debug code - write the stream to debug file /tmp/emf-stream.emf
-        EMFP_DEBUG(int pos = pWMF->Tell();
+        int pos = pWMF->Tell();
         pWMF->Seek(0);
         SvFileStream file( OUString( "/tmp/emf-stream.emf" ), STREAM_WRITE | STREAM_TRUNC );
 
@@ -251,7 +248,9 @@
         file.Flush();
         file.Close();
 
-        pWMF->Seek( pos );)
+        pWMF->Seek( pos );
+#endif
+
     }
     bEMFPlus = true;
 
@@ -265,7 +264,7 @@
 
     OSL_ASSERT(length >= 4);
     // reduce by 32bit length itself, skip in SeekRel if
-    // impossibly unavailble
+    // impossibly unavailable
     sal_uInt32 nRemainder = length >= 4 ? length-4 : length;
 
     const size_t nRequiredHeaderSize = 12;
@@ -277,12 +276,12 @@
         *pWMF >> type >> flags >> size >> dataSize;
         nRemainder -= nRequiredHeaderSize;
 
-        EMFP_DEBUG(printf ("\t\tEMF+ record type: %d\n", type));
+        SAL_INFO ("vcl.emf", "\t\tEMF+ record type: " << std::hex << type);
 
         // GetDC
         if( type == 16388 ) {
             bHaveDC = true;
-            EMFP_DEBUG(printf ("\t\tEMF+ lock DC (device context)\n"));
+            SAL_INFO ("vcl.emf", "\t\tEMF+ lock DC (device context)");
         }
 
         // Get the length of the remaining data of this record based
@@ -454,26 +453,27 @@
         if(  !aBmpSaveList.empty()
           && ( nRecType != EMR_STRETCHBLT )
           && ( nRecType != EMR_STRETCHDIBITS )
-          )
+          ) {
             pOut->ResolveBitmapActions( aBmpSaveList );
+        }
 
         bFlag = sal_False;
 
-        EMFP_DEBUG(printf ("0x%04x-0x%04x record type: %d size: %d\n",(unsigned int) (nNextPos - 
nRecSize),(unsigned int) nNextPos, (int)nRecType,(int) nRecSize));
+        SAL_INFO ("vcl.emf", "0x" << std::hex << (nNextPos - nRecSize) <<  "-0x" << nNextPos << " 
record type: " << std::dec << nRecType << " size: " <<  nRecSize);
 
         if( bEnableEMFPlus && nRecType == EMR_GDICOMMENT ) {
             sal_uInt32 length;
 
             *pWMF >> length;
 
-            EMFP_DEBUG(printf ("\tGDI comment\n\t\tlength: %d\n", (int)length));
+            SAL_INFO("vcl.emf", "\tGDI comment\n\t\tlength: " << length);
 
             if( pWMF->good() && length >= 4 ) {
                 sal_uInt32 id;
 
                 *pWMF >> id;
 
-                EMFP_DEBUG(printf ("\t\tbegin %c%c%c%c id: 0x%x\n", (char)(id & 0xff), (char)((id 
& 0xff00) >> 8), (char)((id & 0xff0000) >> 16), (char)((id & 0xff000000) >> 24), (unsigned int)id));
+                SAL_INFO ("vcl.emf", "\t\tbegin " << (char)(id & 0xff) << (char)((id & 0xff00) >> 
8) << (char)((id & 0xff0000) >> 16) << (char)((id & 0xff000000) >> 24) << " id: 0x" << std::hex << 
id);
 
                 // EMF+ comment (FIXME: BE?)
                 if( id == 0x2B464D45 && nRecSize >= 12 )
@@ -482,7 +482,7 @@
                 else if( id == 0x43494447 && nRecSize >= 12 ) {
                     // TODO: ReadGDIComment()
                 } else {
-                    EMFP_DEBUG(printf ("\t\tunknown id: 0x%x\n",(unsigned int) id));
+                    SAL_INFO ("vcl.emf", "\t\tunknown id: 0x" << std::hex << id);
                 }
             }
         }
@@ -1341,16 +1341,21 @@
 
 sal_Bool EnhWMFReader::ReadHeader()
 {
-    sal_uInt32      nsal_uInt32, nHeaderSize, nPalEntries;
+    sal_uInt32      nType, nSignature, nVersion;
+    sal_uInt32      nHeaderSize, nPalEntries;
     sal_Int32       nLeft, nTop, nRight, nBottom;
 
     // Spare me the METAFILEHEADER here
-    // Reading the METAHEADER
-    *pWMF >> nsal_uInt32 >> nHeaderSize;
-    if ( nsal_uInt32 != 1 )         // Type
+    // Reading the METAHEADER - EMR_HEADER ([MS-EMF] section 2.3.4.2 EMR_HEADER Record Types)
+    *pWMF >> nType >> nHeaderSize;
+    if ( nType != 1 ) { // per [MS-EMF] 2.3.4.2 EMF Header Record Types, type MUST be 0x00000001
+        SAL_WARN("vcl.emf", "EMF header type is not set to 0x00000001 - possibly corrupted file?");
         return sal_False;
+    }
 
-    // bound size
+    // Start reading the EMR_HEADER Header object
+
+    // bound size (RectL object, see [MS-WMF] section 2.2.2.19)
     Rectangle rclBounds;    // rectangle in logical units
     *pWMF >> nLeft >> nTop >> nRight >> nBottom;
     rclBounds.Left() = nLeft;
@@ -1358,7 +1363,7 @@
     rclBounds.Right() = nRight;
     rclBounds.Bottom() = nBottom;
 
-    // picture frame size
+    // picture frame size (RectL object)
     Rectangle rclFrame;     // rectangle in device units 1/100th mm
     *pWMF >> nLeft >> nTop >> nRight >> nBottom;
     rclFrame.Left() = nLeft;
@@ -1366,27 +1371,67 @@
     rclFrame.Right() = nRight;
     rclFrame.Bottom() = nBottom;
 
-    *pWMF >> nsal_uInt32;                               // signature
+    *pWMF >> nSignature;
 
-    if ( nsal_uInt32 != 0x464d4520 )
+    // nSignature MUST be the ASCII characters "FME", see [WS-EMF] 2.2.9 Header Object
+    // and 2.1.14 FormatSignature Enumeration
+    if ( nSignature != 0x464d4520 ) {
+        SAL_WARN("vcl.emf", "EMF\t\tSignature is not 0x464d4520 (\"FME\") - possibly corrupted 
file?");
         return sal_False;
+    }
 
-    *pWMF >> nsal_uInt32;                               // nVersion
+    *pWMF >> nVersion;  // according to [WS-EMF] 2.2.9, this SHOULD be 0x0001000, however
+                        // Microsoft note that not even Windows checks this...
+    if ( nVersion != 0x00010000 ) {
+        SAL_WARN("vcl.emf", "EMF\t\tThis really should be 0x00010000, though not absolutely 
essential...");
+    }
+
     *pWMF >> nEndPos;                                   // size of metafile
     nEndPos += nStartPos;
 
     sal_uInt32 nStrmPos = pWMF->Tell();                 // checking if nEndPos is valid
     pWMF->Seek( STREAM_SEEK_TO_END );
-    if ( pWMF->Tell() < nEndPos )
-        nEndPos = pWMF->Tell();
+    sal_uInt32 nActualFileSize = pWMF->Tell();
+
+    if ( nActualFileSize < nEndPos ) {
+        SAL_WARN("vcl.emf", "EMF\t\tEMF Header object records number of bytes as " << nEndPos
+                            << ", however the file size is actually " << nActualFileSize
+                            << " bytes. Possible file corruption?");
+        nEndPos = nActualFileSize;
+    }
     pWMF->Seek( nStrmPos );
 
     *pWMF >> nRecordCount;
 
-    if ( !nRecordCount )
+    if ( !nRecordCount ) {
+        SAL_WARN("vcl.emf", "EMF\t\tEMF Header object shows record counter as 0! This shouldn't "
+                            "be possible... indicator of possible file corruption?");
         return sal_False;
+    }
 
-    pWMF->SeekRel( 0xc );
+    // the number of "handles", or graphics objects used in the metafile
+
+    sal_uInt16 nHandlesCount;
+    *pWMF >> nHandlesCount;
+
+    // the next 2 bytes are reserved, but according to [MS-EMF] section 2.2.9
+    // it MUST be 0x000 and MUST be ignored... the thing is, having such a specific
+    // value is actually pretty useful in checking if there is possible corruption
+
+    sal_uInt16 nReserved;
+    *pWMF >> nReserved;
+
+    if ( nReserved != 0x0000 ) {
+        SAL_WARN("vcl.emf", "EMF\t\tEMF Header object's reserved field is NOT 0x0000... possible "
+                            "corruption?");
+    }
+
+    // The next 4 bytes is specifies the number of characters in the metafile description
+    // the 4 bytes after that specific the offset from this record that contains the
+    // metafile description... zero means no description string.
+    // For now, we ignore it.
+
+    pWMF->SeekRel( 0x8 );
 
     sal_Int32 nPixX, nPixY, nMillX, nMillY;
     *pWMF >> nPalEntries >> nPixX >> nPixY >> nMillX >> nMillY;

-- 
To view, visit https://gerrit.libreoffice.org/3839
To unsubscribe, visit https://gerrit.libreoffice.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id4ed486b2dd0c6e7bdee67cb344aaaf8e8d98f84
Gerrit-PatchSet: 1
Gerrit-Project: core
Gerrit-Branch: master
Gerrit-Owner: Chris Sherlock <chris.sherlock79@gmail.com>