On Mon, 19 Nov 2007, Alexandre Julliard wrote: I've been looking into this, and I'm afraid I'll need some help to proceed. If you look at the code and my original patch below, you will see that I removed four conditions which were noops, that is, the compiler should (and could) simply remove them. This is what my patch did at the source level. If we want to add some input checking, I assume you would like to check that these values are not too large? (They cannot be negative, so the only range checking we can do is on the upper end.) How should this look like? Any specific upper bounds you have in mind? Or did I simply fail to explain my original patch, that is, convey the point that this actually will not change program behavior? 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 28 Nov 2007 23:18:34 -0000 @@ -1670,9 +1670,7 @@ BOOL WINAPI PlayEnhMetaFileRecord( 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) && + if ( !( ((lpCreate->offBmi +lpCreate->cbBmi ) <= mr->nSize) && ((lpCreate->offBits+lpCreate->cbBits) <= mr->nSize) ) ) { ERR("Invalid EMR_CREATEDIBPATTERNBRUSHPT record\n");

On Sun, 2 Dec 2007, Alexandre Julliard wrote: 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;