Re: [2/5] strmbase: Add validation checks when updating source rectangle.

[Andrew Eikum]

On Sun, Nov 20, 2016 at 03:33:22PM +0900, Akihiro Sagawa wrote:
> @@ -53,6 +53,26 @@ HRESULT WINAPI BaseControlVideo_Destroy(BaseControlVideo *pControlVideo)
>      return BaseDispatch_Destroy(&pControlVideo->baseDispatch);
>  }
>
> +static HRESULT BaseControlVideoImpl_CheckSourceRect(BaseControlVideo *This, RECT *pSourceRect)
> +{
> - LONG VideoWidth, VideoHeight;
> - HRESULT hr;
> -
> - if (pSourceRect->top < 0 || pSourceRect->left < 0 ||
> -
>    pSourceRect->top >= pSourceRect->bottom ||
> -
>    pSourceRect->left >= pSourceRect->right)
> -
>    return E_INVALIDARG;

I'm not opposed to spelling it out, but you could use IsRectEmpty()
here instead.
> -
> - hr = BaseControlVideoImpl_GetVideoSize((IBasicVideo *)This, &VideoWidth, &VideoHeight);
> - if (FAILED(hr))
> -
>    return hr;
> -
> - if (pSourceRect->bottom > VideoHeight || pSourceRect->right > VideoWidth)
> -
>    return E_INVALIDARG;
> -

Shouldn't these use (bottom - top) and (right - left)?

Andrew

[2/5] strmbase: Add validation checks when updating source rectangle.

[Akihiro Sagawa]

On Mon, 21 Nov 2016 10:19:59 -0600, Andrew Eikum wrote:
> On Sun, Nov 20, 2016 at 03:33:22PM +0900, Akihiro Sagawa wrote:
> > @@ -53,6 +53,26 @@ HRESULT WINAPI BaseControlVideo_Destroy(BaseControlVideo *pControlVideo)
> >      return BaseDispatch_Destroy(&pControlVideo->baseDispatch);
> >  }
> >
> > +static HRESULT BaseControlVideoImpl_CheckSourceRect(BaseControlVideo *This, RECT *pSourceRect)
> > +{
> > - LONG VideoWidth, VideoHeight;
> > - HRESULT hr;
> > -
> > - if (pSourceRect->top < 0 || pSourceRect->left < 0 ||
> > -
> >    pSourceRect->top >= pSourceRect->bottom ||
> > -
> >    pSourceRect->left >= pSourceRect->right)
> > -
> >    return E_INVALIDARG;
>
> I'm not opposed to spelling it out, but you could use IsRectEmpty()
> here instead.

Thanks for reviewing. I sent v2 patch set based on your helpful comments.
It also includes fix of underlying copy-paste error.
> > -
> > - hr = BaseControlVideoImpl_GetVideoSize((IBasicVideo *)This, &VideoWidth, &VideoHeight);
> > - if (FAILED(hr))
> > -
> >    return hr;
> > -
> > - if (pSourceRect->bottom > VideoHeight || pSourceRect->right > VideoWidth)
> > -
> >    return E_INVALIDARG;
> > -
>
> Shouldn't these use (bottom - top) and (right - left)?

I'd like to say no at this point because native doesn't accept source
rectangle that points outside of the original video area.
tests/filtergraph.c's line 94 and 104 test this case.

Akihiro Sagawa

[2/5] strmbase: Add validation checks when updating source rectangle.

[Andrew Eikum]

On Fri, Nov 25, 2016 at 01:10:08AM +0900, Akihiro Sagawa wrote:
> Thanks for reviewing. I sent v2 patch set based on your helpful comments.
> It also includes fix of underlying copy-paste error.

New patches look good, thanks for working on this!

Andrew