Common copy_pixels() helper already performs theses checks, and the buffer size check is wrong.
Signed-off-by: Dmitry Timoshkov dmitry@baikal.ru --- dlls/windowscodecs/tests/tiffformat.c | 2 -- dlls/windowscodecs/tiffformat.c | 9 --------- 2 files changed, 11 deletions(-)
diff --git a/dlls/windowscodecs/tests/tiffformat.c b/dlls/windowscodecs/tests/tiffformat.c index 8da843bdcc..ff9c4b9a74 100644 --- a/dlls/windowscodecs/tests/tiffformat.c +++ b/dlls/windowscodecs/tests/tiffformat.c @@ -598,11 +598,9 @@ static void test_tiff_24bpp(void) ok(hr == E_INVALIDARG, "CopyPixels(%u) should fail: %#x\n", stride, hr); else { -todo_wine_if(stride > 3) ok(hr == S_OK, "CopyPixels(%u) error %#x\n", stride, hr);
for (i = 0; i < sizeof(data); i++) -todo_wine_if(stride > 3) ok(data[i] == expected_data[i], "%u: expected %02x, got %02x\n", i, expected_data[i], data[i]); } } diff --git a/dlls/windowscodecs/tiffformat.c b/dlls/windowscodecs/tiffformat.c index 93af5594a7..2d265e8694 100644 --- a/dlls/windowscodecs/tiffformat.c +++ b/dlls/windowscodecs/tiffformat.c @@ -1080,7 +1080,6 @@ static HRESULT WINAPI TiffFrameDecode_CopyPixels(IWICBitmapFrameDecode *iface, WICRect rc; HRESULT hr=S_OK; BYTE *dst_tilepos; - UINT bytesperrow; WICRect rect;
TRACE("(%p,%s,%u,%u,%p)\n", iface, debug_wic_rect(prc), cbStride, cbBufferSize, pbBuffer); @@ -1100,14 +1099,6 @@ static HRESULT WINAPI TiffFrameDecode_CopyPixels(IWICBitmapFrameDecode *iface, return E_INVALIDARG; }
- bytesperrow = ((This->decode_info.bpp * prc->Width)+7)/8; - - if (cbStride < bytesperrow) - return E_INVALIDARG; - - if ((cbStride * prc->Height) > cbBufferSize) - return E_INVALIDARG; - min_tile_x = prc->X / This->decode_info.tile_width; min_tile_y = prc->Y / This->decode_info.tile_height; max_tile_x = (prc->X+prc->Width-1) / This->decode_info.tile_width;
This could lead to a partial read with an E_INVALIDARG return, if the next tile boundary is within cbBufferSize. That seems wrong (and a bit dangerous for code freeze).
On Wed, Jan 9, 2019 at 12:51 AM Dmitry Timoshkov dmitry@baikal.ru wrote:
Common copy_pixels() helper already performs theses checks, and the buffer size check is wrong.
Signed-off-by: Dmitry Timoshkov dmitry@baikal.ru
dlls/windowscodecs/tests/tiffformat.c | 2 -- dlls/windowscodecs/tiffformat.c | 9 --------- 2 files changed, 11 deletions(-)
diff --git a/dlls/windowscodecs/tests/tiffformat.c b/dlls/windowscodecs/tests/tiffformat.c index 8da843bdcc..ff9c4b9a74 100644 --- a/dlls/windowscodecs/tests/tiffformat.c +++ b/dlls/windowscodecs/tests/tiffformat.c @@ -598,11 +598,9 @@ static void test_tiff_24bpp(void) ok(hr == E_INVALIDARG, "CopyPixels(%u) should fail: %#x\n", stride, hr); else { -todo_wine_if(stride > 3) ok(hr == S_OK, "CopyPixels(%u) error %#x\n", stride, hr);
for (i = 0; i < sizeof(data); i++)
-todo_wine_if(stride > 3) ok(data[i] == expected_data[i], "%u: expected %02x, got %02x\n", i, expected_data[i], data[i]); } } diff --git a/dlls/windowscodecs/tiffformat.c b/dlls/windowscodecs/tiffformat.c index 93af5594a7..2d265e8694 100644 --- a/dlls/windowscodecs/tiffformat.c +++ b/dlls/windowscodecs/tiffformat.c @@ -1080,7 +1080,6 @@ static HRESULT WINAPI TiffFrameDecode_CopyPixels(IWICBitmapFrameDecode *iface, WICRect rc; HRESULT hr=S_OK; BYTE *dst_tilepos;
UINT bytesperrow; WICRect rect;
TRACE("(%p,%s,%u,%u,%p)\n", iface, debug_wic_rect(prc), cbStride, cbBufferSize, pbBuffer);
@@ -1100,14 +1099,6 @@ static HRESULT WINAPI TiffFrameDecode_CopyPixels(IWICBitmapFrameDecode *iface, return E_INVALIDARG; }
- bytesperrow = ((This->decode_info.bpp * prc->Width)+7)/8;
- if (cbStride < bytesperrow)
return E_INVALIDARG;
- if ((cbStride * prc->Height) > cbBufferSize)
return E_INVALIDARG;
- min_tile_x = prc->X / This->decode_info.tile_width; min_tile_y = prc->Y / This->decode_info.tile_height; max_tile_x = (prc->X+prc->Width-1) / This->decode_info.tile_width;
-- 2.20.1
Vincent Povirk vincent@codeweavers.com wrote:
This could lead to a partial read with an E_INVALIDARG return, if the next tile boundary is within cbBufferSize. That seems wrong (and a bit dangerous for code freeze).
Thanks for the review. Feel free to send a different fix.
I guess that 3/3 could be accepted regarless.