On Sun, 2 Dec 2007, Alexandre Julliard wrote:
I'm aware of that, but the purpose of having these warnings is to spot bugs, and when you find a bug you have to fix it. Yes, the checks currently don't work, so they should be made to work, not removed. As the comment says, you have to check that offsets and sizes are contained within the record.
Doesn't the following part of the checks work and ensure that already?
if ( !( ((lpCreate->offBmi +lpCreate->cbBmi ) <= mr->nSize) && ((lpCreate->offBits+lpCreate->cbBits) <= mr->nSize) ) )
My patch only removes the half of the original check which is a noop (since the current code misses the fact that these variables always will be >= 0 by virtue of their type).
Ahhh! A lightbulb goes on. Since this is input from the outside, and thus completely out of our control, you are worried about overflows, that is, the sum of the two values (offset and size) being within range, but not the individual parts.
Is this what you've been after? :-)
Updated patch below. (Now I only wonder whether the <= in the original code shouldn't have been <, and thus the > in my code shouldn't be >=, but that's a separate issue.)
Gerald
Index: dlls/gdi32/enhmetafile.c =================================================================== RCS file: /home/wine/wine/dlls/gdi32/enhmetafile.c,v retrieving revision 1.6 diff -u -3 -p -r1.6 enhmetafile.c --- dlls/gdi32/enhmetafile.c 3 Aug 2007 13:06:43 -0000 1.6 +++ dlls/gdi32/enhmetafile.c 2 Dec 2007 23:32:50 -0000 @@ -1669,11 +1669,15 @@ BOOL WINAPI PlayEnhMetaFileRecord( const EMRCREATEDIBPATTERNBRUSHPT *lpCreate = (const EMRCREATEDIBPATTERNBRUSHPT *)mr; LPVOID lpPackedStruct;
- /* check that offsets and data are contained within the record */ - if ( !( (lpCreate->cbBmi>=0) && (lpCreate->cbBits>=0) && - (lpCreate->offBmi>=0) && (lpCreate->offBits>=0) && - ((lpCreate->offBmi +lpCreate->cbBmi ) <= mr->nSize) && - ((lpCreate->offBits+lpCreate->cbBits) <= mr->nSize) ) ) + /* Check that offsets and data are contained within the record. + * The first four checks are not redundant -- think overflow! + */ + if ( lpCreate->offBmi > mr->nSize + || lpCreate->cbBmi > mr->nSize + || lpCreate->offBits > mr->nSize + || lpCreate->cbBits > mr->nSize + || lpCreate->offBmi +lpCreate->cbBmi > mr->nSize + || lpCreate->offBits+lpCreate->cbBits > mr->nSize ) { ERR("Invalid EMR_CREATEDIBPATTERNBRUSHPT record\n"); break;