Copying Windows EMFs in code

A worked example in asynchronous code paranoia.

Our application contains diagrams, which are stored in a configuration file as embedded metafiles. Part of the application allows new diagrams to be added to the configuration file. A colleague commented recently that our application seemed to lock the .emf file containing a picture being added – once the picture was added, you couldn’t change the .emf until you closed our application down.

When I started investigating, I discovered we call GetEnhMetaFile( filename ) when loading the metafile to add to the configuration file.  We call GetEnhMetaFileBits to copy the binary data for saving into the config file.  And we call SetEnhMetaFileBits when loading the binary data back from the config file.

And that’s it. No clean-up, ever.

So no wonder the file is still locked. (Unfortunately, although our automated tests spot memory leaks, they don’t spot resource leaks yet.  Any suggestions?)

After poking around a bit (the engineer who wrote that section is no longer with us), I discovered it’s not clear when it is safe to delete the metafile, because metafile handles are shared around so that there’s no way of knowing whether something else is referring to the same metafile, or even where it came from in the first place.

So the simplest way to deal with this is to take a copy of the metafile when assigning it to a diagram and then close the source handle at the place itwas opened.  That way the lifetime and ownership is clear.  The loading routine is responsible for its copy, and the diagram is responsible for its copy.  The delete is clearly near the allocation.

How do you copy a metafile?  Hmmm.  There’s no CopyEnhMetaFile function.  We’ll need to get the bits from the source and set them to the destination.

GetEnhMetaFileBits takes a handle, a buffer and a buffer length and fills in the buffer from the metafile.  Great…but how big should the buffer be?  Simple – you call it first with the buffer set to NULL and it returns the size you need.  Allocate a buffer of the right size, call it again, and Bob’s your uncle.  You’ve now got the binary data in a buffer which is in the perfect format for calling SetEnhMetaFileBits, and you know exactly how long it should be.

// CopyMetaFile version 1 - don't use this one!
WM_RESULT CopyMetaFile( HENHMETAFILE picture, HENHMETAFILE *phCopy )
{
  WM_RESULT       result;
  HENHMETAFILE    hMetaFile = NULL;
  UINT            buflen = 0;
  LPBYTE          buffer = NULL;
  UINT            retval;

  buflen = GetEnhMetaFileBits( picture, 0, NULL );
  if ( !buflen )
  {
    result = WMR_BAD_FILE_FORMAT;
    goto error;
  }
  buffer = (LPBYTE) malloc( buflen );
  if ( !buffer )
  {
    result = WMR_RESOURCE_FAIL;
    goto error;
  }
  retval = GetEnhMetaFileBits( picture, buflen, buffer );
  if ( !retval )
  {
    result = WMR_BAD_FILE_FORMAT;
    goto error;
  }
  hMetaFile = SetEnhMetaFileBits( buflen, buffer );
  if ( !hMetaFile )
  {
    result = WMR_BAD_FILE_FORMAT;
    goto error;
  }

  *phCopy = hMetaFile;
  return WMR_SUCCESS;

error:
  return result;
}

But what about the buffer we’ve just allocated.  Who owns that?  Is it still needed by the metafile, or has the data been copied out of it? Testing with this definitely shows a memory leak, but maybe the metafile still refers to it and freeing the memory again would be dangerous.

MSDN was silent on the topic, a quick web search found nothing either.   The only option was to try it and see.  So free the buffer again after calling SetEnhMetaFileBits.  Quick compile, run the program – it all seems to work.  We still get the pictures, and there’s no crash.

But maybe we got lucky – maybe the memory hasn’t yet been reused, and the program is just accessing the stale, deleted, but not yet cleaned up memory, and it will crash in half a hour when something else gets given that memory during an allocation.  That would be a horrible bug to track down.  How can we make certain?

I know – let’s corrupt the memory immediately before we delete it.  A simple memset to 0 should be enough to wipe out any metafile information, and that would be instantly visible in the application as all the metafiles would just be blank.  Compile, run…no everything still works perfectly.

Okay, maybe I’ve not managed to corrupt the memory properly. (You may think I’m being overly cautious here, but with asynchronous programming, paranoia is definitely your friend.)  Maybe Windows somehow manages to cope, while still needing the memory there.  Maybe the build didn’t pick up the change properly. How can I check that?

Well, what happens if I corrupt the memory before calling SetEnhMetaFileBits?  Let’s move the memset to before the call.  This time no pictures, and in fact the app crashes.  So we’re definitely making a difference.

Move the memset back after the SetEnhMetaFileBits and try again.  Does this work again?  If so, we’re on a winner.  And indeed everything works perfectly again.  So SetEnhMetaFileBits does take a copy of the memory, and we can free our temporary buffer straight away.

So the final routine is as follows:

////////////////////////////////////////////////////////////////////////////
///
//  Function: CopyMetaFile
///
/// @brief  Creates a copy of the metafile.
///
/// Written by Ian Brockbank.  Provided with no warranty whatsoever.
/// Feel free to use for any purpose you see fit.
///
/// @param  picture     Metafile to copy.
///
/// @retval WMR_SUCCESS             Succeeded.
/// @retval WMR_BAD_FILE_FORMAT     The metafile couldn't be parsed.
/// @retval WMR_RESOURCE_FAIL       Couldn't allocate a new buffer.
///
////////////////////////////////////////////////////////////////////////////
WM_RESULT CopyMetaFile( HENHMETAFILE picture, HENHMETAFILE *phCopy )
{
  WM_RESULT       result;
  HENHMETAFILE    hMetaFile = NULL;
  UINT            buflen = 0;
  LPBYTE          buffer = NULL;
  UINT            retval;

  buflen = GetEnhMetaFileBits( picture, 0, NULL );
  if ( !buflen )
  {
    result = WMR_BAD_FILE_FORMAT;
    goto error;
  }
  buffer = (LPBYTE) malloc( buflen );
  if ( !buffer )
  {
    result = WMR_RESOURCE_FAIL;
    goto error;
  }
  retval = GetEnhMetaFileBits( picture, buflen, buffer );
  if ( !retval )
  {
    result = WMR_BAD_FILE_FORMAT;
    goto error;
  }
  hMetaFile = SetEnhMetaFileBits( buflen, buffer );
  if ( !hMetaFile )
  {
    result = WMR_BAD_FILE_FORMAT;
    goto error;
  }

  //
  // SetEnhMetaFileBits has taken a copy of the buffer, so
  // release our local copy.
  //
  free( buffer );
  buffer = NULL;

  *phCopy = hMetaFile;
  return WMR_SUCCESS;

error:
  // Clean up.
  if ( hMetaFile )
  {
    DeleteEnhMetaFile( hMetaFile );
    hMetaFile = NULL;
  }
  if ( buffer )
  {
    free( buffer );
    buffer = NULL;
  }

  return result;
}

So there you go.  SetEnhMetaFileBits takes a copy of the data buffer passed in, and the caller can release the buffer immediately after the call.

This code comes with no warranties, but feel free to copy and reuse it as you see fit. It worked for me, but I make no guarantees it will also work for you, and maybe there are other holes I didn’t think of which will catch me in future. If you do find a problem with the code (other than stylistic), please let me know and I’ll gladly try to fix it.  Otherwise, enjoy!

Advertisements
This entry was posted in Coding and tagged , , , . Bookmark the permalink.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s