Re: [3/7] dxdiagn: Fix string copy behavior with an excessively short buffer in IDxDiagContainer::EnumChildContainerNames. (try 2)

[Vitaliy Margolen]

On 03/14/2010 11:04 AM, Andrew Nguyen wrote:
> --- a/dlls/dxdiagn/container.c
> +++ b/dlls/dxdiagn/container.c
> @@ -97,12 +97,10 @@ static HRESULT WINAPI IDxDiagContainerImpl_EnumChildContainerNames(PDXDIAGCONTAI
>
> p = This->subContainers;
>    while (NULL != p) {
> -    if (dwIndex == i) {  
> -      if (cchContainer <= strlenW(p->contName)) {
> -	return DXDIAG_E_INSUFFICIENT_BUFFER;
> -      }
> +    if (dwIndex == i) {
>        lstrcpynW(pwszContainer, p->contName, cchContainer);
> -      return S_OK;
> +      return (cchContainer <= strlenW(p->contName)) ?
> +              DXDIAG_E_INSUFFICIENT_BUFFER : S_OK;
>      }

You sure it's "<="? You still need one more character for \0.

Vitaliy.

[3/7] dxdiagn: Fix string copy behavior with an excessively short buffer in IDxDiagContainer::EnumChildContainerNames. (try 2)

[Andrew Nguyen]

On Sun, Mar 14, 2010 at 3:08 PM, Vitaliy Margolen
<wine-devel(a)kievinfo.com> wrote:
> On 03/14/2010 11:04 AM, Andrew Nguyen wrote:
> > --- a/dlls/dxdiagn/container.c
> > +++ b/dlls/dxdiagn/container.c
> > @@ -97,12 +97,10 @@ static HRESULT WINAPI IDxDiagContainerImpl_EnumChildContainerNames(PDXDIAGCONTAI
> >
> >    p = This->subContainers;
> >    while (NULL != p) {
> > -    if (dwIndex == i) {
> > -      if (cchContainer <= strlenW(p->contName)) {
> > -     return DXDIAG_E_INSUFFICIENT_BUFFER;
> > -      }
> > +    if (dwIndex == i) {
> >        lstrcpynW(pwszContainer, p->contName, cchContainer);
> > -      return S_OK;
> > +      return (cchContainer <= strlenW(p->contName)) ?
> > +              DXDIAG_E_INSUFFICIENT_BUFFER : S_OK;
> >      }
>
> You sure it's "<="? You still need one more character for \0.
>
> Vitaliy.

Yes, because if cchContainer is the same value as what strlenW
returns, that means that the buffer can't store the null terminator,
so the function would correctly report DXDIAG_E_INSUFFICIENT_BUFFER.
lstrcpyn takes care of terminating the string at the appropriate
position.

[3/7] dxdiagn: Fix string copy behavior with an excessively short buffer in IDxDiagContainer::EnumChildContainerNames. (try 2)

[Vitaliy Margolen]

On 03/14/2010 03:14 PM, Andrew Nguyen wrote:
> On Sun, Mar 14, 2010 at 3:08 PM, Vitaliy Margolen
> <wine-devel(a)kievinfo.com> wrote:
> > On 03/14/2010 11:04 AM, Andrew Nguyen wrote:
> > > +    if (dwIndex == i) {
> > >        lstrcpynW(pwszContainer, p->contName, cchContainer);
> > > -      return S_OK;
> > > +      return (cchContainer <= strlenW(p->contName)) ?
> > > +              DXDIAG_E_INSUFFICIENT_BUFFER : S_OK;
> > >      }
> >
> > You sure it's "<="? You still need one more character for \0.
>
> Yes, because if cchContainer is the same value as what strlenW
> returns, that means that the buffer can't store the null terminator,
> so the function would correctly report DXDIAG_E_INSUFFICIENT_BUFFER.
> lstrcpyn takes care of terminating the string at the appropriate
> position.

You right.

Vitaliy