Re: [PATCH 1/4] ole32: Add support for loading enhmetafiles from presentation and contents streams to data cache.

[Huw Davies]

On Tue, Mar 27, 2018 at 11:39:58AM -0500, Sergio Gómez Del Real wrote:
> +static HRESULT load_emf( DataCacheEntry *cache_entry, IStream *stm )
> +{
> +
> +    if (cache_entry->load_stream_num != STREAM_NUMBER_CONTENTS)
> +    {
> +        HMETAFILEPICT hmfpict;
> +        METAFILEPICT *mfpict;
> +
> +        load_mf_pict(cache_entry, stm);
> +        hmfpict = cache_entry->stgmedium.u.hMetaFilePict;
> +        synthesize_emf(hmfpict, &cache_entry->stgmedium);
> +        mfpict = GlobalLock(hmfpict);
> +        DeleteMetaFile(mfpict->hMF);
> +        GlobalFree(hmfpict);

There's no error checking in this block at all.  Also the clean way to
do this would be declare a STGMEDIUM temporary variable and pass the
address of that as the 2nd param of synthesize_emf().  Then call
ReleaseStgMedium() on the cache_entry's stgmedium and copy the
temporary stgmedium to the cache_entry.
> +    }
> +    else
> +    {
> +        HRESULT hr;
> +        STATSTG stat;
> +        void *data;
> +        DWORD size_bits;
> +        ULONG read;
> +
> +        hr = IStream_Stat( stm, &stat, STATFLAG_NONAME );
> +        if (FAILED( hr )) return hr;
>
> +        data = HeapAlloc( GetProcessHeap(), 0, stat.cbSize.u.LowPart );
> +        if (!data) return E_OUTOFMEMORY;
> +
> +        hr = IStream_Read( stm, data, stat.cbSize.u.LowPart, &read );
> +        if (hr != S_OK || read != stat.cbSize.u.LowPart) hr = E_FAIL;
> +

Just setting hr to E_FAIL isn't enough here, right?
> +        size_bits = read - sizeof(DWORD) - sizeof(ENHMETAHEADER);
> +        if (size_bits <= 0)
> +        {
> +            HeapFree( GetProcessHeap(), 0, data );
> +            return E_FAIL;
> +        }
> +        cache_entry->stgmedium.u.hEnhMetaFile = SetEnhMetaFileBits( size_bits, (BYTE *)data + (read - size_bits) );
> +        cache_entry->stgmedium.tymed = TYMED_ENHMF;
> +        cache_entry->stgmedium.pUnkForRelease = NULL;

You leak data here.
> +    }
> +
> +    return S_OK;
>  }
>
> /************************************************************************
> @@ -736,6 +805,10 @@ static HRESULT DataCacheEntry_LoadData(DataCacheEntry *cache_entry, IStorage *st
>          hr = load_dib( cache_entry, stm );
>          break;
>
> +    case CF_ENHMETAFILE:
> +        hr = load_emf( cache_entry, stm );
> +        break;
> +
>      default:
>          FIXME( "Unimplemented clip format %x\n", cache_entry->fmtetc.cfFormat );
>          hr = E_NOTIMPL;
> @@ -1118,30 +1191,6 @@ static HRESULT synthesize_bitmap( HGLOBAL dib, STGMEDIUM *med )
>      return hr;
>  }
>
> -static HRESULT synthesize_emf( HMETAFILEPICT data, STGMEDIUM *med )
> -{
> -    METAFILEPICT *pict;
> -    HRESULT hr = E_FAIL;
> -    UINT size;
> -    void *bits;
> -
> -    if (!(pict = GlobalLock( data ))) return hr;
> -
> -    size = GetMetaFileBitsEx( pict->hMF, 0, NULL );
> -    if ((bits = HeapAlloc( GetProcessHeap(), 0, size )))
> -    {
> -        GetMetaFileBitsEx( pict->hMF, size, bits );
> -        med->u.hEnhMetaFile = SetWinMetaFileBits( size, bits, NULL, pict );
> -        HeapFree( GetProcessHeap(), 0, bits );
> -        med->tymed = TYMED_ENHMF;
> -        med->pUnkForRelease = NULL;
> -        hr = S_OK;
> -    }
> -
> -    GlobalUnlock( data );
> -    return hr;
> -}
> -
>  static HRESULT DataCacheEntry_SetData(DataCacheEntry *cache_entry,
>                                        const FORMATETC *formatetc,
>                                        STGMEDIUM *stgmedium,
> -- 
> 2.14.1