Re: Remove four useless checks in dlls/gdi32/enhmetafile.c (RESEND)
Gerald Pfeifer <gerald(a)pfeifer.com> writes:
One of the really uncontested ones, I think. ;-)
---------- Forwarded message ---------- From: Gerald Pfeifer <gerald(a)pfeifer.com> To: <wine-patches(a)winehq.org> Date: Thu, 1 Nov 2007 16:10:03 +0100 (CET) Subject: Remove four useless checks in dlls/gdi32/enhmetafile.c
The members of EMRCREATEDIBPATTERNBRUSHPT are of type DWORD, so comparing them for >= 0 is a noop which always evaluates to true.
Gerald
ChangeLog: Remove four unnecessary comparisions of DWORD variables for >= 0.
Validating the record is not unnecessary, it just needs to be done correctly by checking for wraparounds etc. -- Alexandre Julliard julliard(a)winehq.org
On Mon, 19 Nov 2007, Alexandre Julliard wrote:
The members of EMRCREATEDIBPATTERNBRUSHPT are of type DWORD, so comparing them for >= 0 is a noop which always evaluates to true.
Gerald
ChangeLog: Remove four unnecessary comparisions of DWORD variables for >= 0. Validating the record is not unnecessary, it just needs to be done correctly by checking for wraparounds etc.
I had expected this comment for a different patch of mine. In dlls/gdi32/enhmetafile.c we are just reading existing records, so I'm not sure what you have in mind here? Gerald
Gerald Pfeifer <gerald(a)pfeifer.com> writes:
I had expected this comment for a different patch of mine. In dlls/gdi32/enhmetafile.c we are just reading existing records, so I'm not sure what you have in mind here?
The records usually come from an external file, so they have to be validated (not that this is done correctly everywhere, but we should move towards more validation, not less). -- Alexandre Julliard julliard(a)winehq.org
On Mon, 19 Nov 2007, Alexandre Julliard wrote:
I had expected this comment for a different patch of mine. In dlls/gdi32/enhmetafile.c we are just reading existing records, so I'm not sure what you have in mind here? The records usually come from an external file, so they have to be validated (not that this is done correctly everywhere, but we should move towards more validation, not less).
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");
Gerald Pfeifer <gerald(a)pfeifer.com> writes:
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?
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. -- Alexandre Julliard julliard(a)winehq.org
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;
Gerald Pfeifer <gerald(a)pfeifer.com> writes:
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? :-)
It's closer, but overflow should be treated as an error even if the result is within range.
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.)
It depends if you are testing a position or a size. Please spend some more time thinking about it, and send a patch with the correct checks. -- Alexandre Julliard julliard(a)winehq.org
participants (2)
-
Alexandre Julliard -
Gerald Pfeifer