[PATCH 2/3] windowscodecs: Remove redundant checks from the TIFF decoder's CopyPixels implementation.

[Dmitry Timoshkov]

Common copy_pixels() helper already performs theses checks, and the buffer
size check is wrong.

Signed-off-by: Dmitry Timoshkov <dmitry(a)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]

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(a)baikal.ru> wrote:
> Common copy_pixels() helper already performs theses checks, and the buffer
> size check is wrong.
>
> Signed-off-by: Dmitry Timoshkov <dmitry(a)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

[Dmitry Timoshkov]

Vincent Povirk <vincent(a)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.

-- 
Dmitry.