Re: version: Fix wrong length reported when selecting a block. (resend)

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2015-02-12 um 20:15 schrieb Mark Jansen:
> LPWSTR lpSubBlockW;
> +        UINT dum = 0;
> +        if (!puLen)
> +            puLen = &dum;
>
> len  = MultiByteToWideChar(CP_ACP, 0, lpSubBlock, -1, NULL, 0);
>          lpSubBlockW = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> @@ -965,7 +968,7 @@ BOOL WINAPI VerQueryValueA( LPCVOID pBlock, LPCSTR lpSubBlock,
>
> HeapFree(GetProcessHeap(), 0, lpSubBlockW);
>
> -        if (ret && isText)
> +        if (ret && isText && *puLen)

I am afraid I have no idea what this patch is doing. I am not even sure
I understand the original function properly :-( . VerQueryValueA seems
like an A to W wrapper for VersionInfo32_QueryValue, but with the quirky
behavior that it appends the ascii string after the unicode string.

Can you elaborate a bit on what exactly goes wrong? What are the input
values, and what does VersionInfo32_QueryValue return?

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJU3dGbAAoJEN0/YqbEcdMwpwEP+wabxLJtbw7O7enIEu21l8ls
m6vxZr1PXP5RnTmOs4nk/QmWT4Oa68qY2EBdArZ79WwyVULLI7BjNJ1aLS67dw/Z
l6Dk9fh8orxoD4HznH0o7PxYrWNrQ1nJ2jf39k+TO4DbSHMZB/eGXnGUZxVFwKVf
H7yNxmoSISC0aWgQP0/QZF+AhtsJ5TLIWtNyJULpmTnZZGu9z94Ukftd9gDL+tZU
ezFsqxdDhpy0nirb4ky/Yq77jXQtsCmz/pCNi7ntYuQqY5fEmtJQMAtpfOQhdG5h
XP1Py7qFTu/RgqnBsO1Z3INRyKk7NS4BsPvULcLsk4WbDvoTGEqa1WwHwqHY4Dng
IDHagE938Xkc8CPiHGzqJG6WDdPPkFiRzqM+2HH87ihJ2uAaVZcK/V46lGiJCx8q
jDY+DlcDyZRmw28+fjwzOjPQHZx/jH2X3wLIpkliM5Ja03Ocb27Aaud9DEQ6H0vw
0VbD2Db6oOrPBv3oyDmZhI+NTPgvsUoaH6sNE+3cIEwBWpCKkOJMzoDZ6EiqWVqj
jvJklcVcOn7nfUBH0YCfnG5szQokMnfcnZaOZxzkjWnPvwJbfC36eLQyESkbtbKf
Y2Z+mvbWslkhdeR0jKAmQNwfDuFcfvZflinem3U9xJAqBKEmicLeqJ0dtwUIJyC7
TnJMMCP14EDaX0DZGHw3
=94Ql
-----END PGP SIGNATURE-----

version: Fix wrong length reported when selecting a block. (resend)

[Mark Jansen]

Hello,

VersionInfo32_QueryValue seeks the requested element in a resource,
and the check after that (which i patched) asks if this succeeded, and
if it succeeded if it is text.
Now there are some rare case where it succeeds, and where it is text
but the length is actually 0.
The data at this point should not matter, but because the check did
not take into account the length returned, and blindly fed the data to
WideCharToMultiByte (which uses some pointer trickery that in this
very specific case is wrong to calculate the length), the result is
VerQueryValueA telling you that it just found you x bytes of data,
where it should have reported 0. (x depending on the data encountered
at the current read position, usually 0 so usually it accidentally
gives the correct value back.

The 'dum' i introduced is purely to make sure the function doesnt trip
if someone gives it a NULL pointer.

regards,

On Fri, Feb 13, 2015 at 11:27 AM, Stefan Dösinger
<stefandoesinger(a)gmail.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Am 2015-02-12 um 20:15 schrieb Mark Jansen:
> > LPWSTR lpSubBlockW;
> > +        UINT dum = 0;
> > +        if (!puLen)
> > +            puLen = &dum;
> >
> > len  = MultiByteToWideChar(CP_ACP, 0, lpSubBlock, -1, NULL, 0);
> >          lpSubBlockW = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> > @@ -965,7 +968,7 @@ BOOL WINAPI VerQueryValueA( LPCVOID pBlock, LPCSTR lpSubBlock,
> >
> > HeapFree(GetProcessHeap(), 0, lpSubBlockW);
> >
> > -        if (ret && isText)
> > +        if (ret && isText && *puLen)
>
> I am afraid I have no idea what this patch is doing. I am not even sure
> I understand the original function properly :-( . VerQueryValueA seems
> like an A to W wrapper for VersionInfo32_QueryValue, but with the quirky
> behavior that it appends the ascii string after the unicode string.
>
> Can you elaborate a bit on what exactly goes wrong? What are the input
> values, and what does VersionInfo32_QueryValue return?
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQIcBAEBAgAGBQJU3dGbAAoJEN0/YqbEcdMwpwEP+wabxLJtbw7O7enIEu21l8ls
> m6vxZr1PXP5RnTmOs4nk/QmWT4Oa68qY2EBdArZ79WwyVULLI7BjNJ1aLS67dw/Z
> l6Dk9fh8orxoD4HznH0o7PxYrWNrQ1nJ2jf39k+TO4DbSHMZB/eGXnGUZxVFwKVf
> H7yNxmoSISC0aWgQP0/QZF+AhtsJ5TLIWtNyJULpmTnZZGu9z94Ukftd9gDL+tZU
> ezFsqxdDhpy0nirb4ky/Yq77jXQtsCmz/pCNi7ntYuQqY5fEmtJQMAtpfOQhdG5h
> XP1Py7qFTu/RgqnBsO1Z3INRyKk7NS4BsPvULcLsk4WbDvoTGEqa1WwHwqHY4Dng
> IDHagE938Xkc8CPiHGzqJG6WDdPPkFiRzqM+2HH87ihJ2uAaVZcK/V46lGiJCx8q
> jDY+DlcDyZRmw28+fjwzOjPQHZx/jH2X3wLIpkliM5Ja03Ocb27Aaud9DEQ6H0vw
> 0VbD2Db6oOrPBv3oyDmZhI+NTPgvsUoaH6sNE+3cIEwBWpCKkOJMzoDZ6EiqWVqj
> jvJklcVcOn7nfUBH0YCfnG5szQokMnfcnZaOZxzkjWnPvwJbfC36eLQyESkbtbKf
> Y2Z+mvbWslkhdeR0jKAmQNwfDuFcfvZflinem3U9xJAqBKEmicLeqJ0dtwUIJyC7
> TnJMMCP14EDaX0DZGHw3
> =94Ql
> -----END PGP SIGNATURE-----

version: Fix wrong length reported when selecting a block. (resend)

[Matteo Bruni]

2015-02-13 20:41 GMT+01:00 Mark Jansen <learn0more+wine(a)gmail.com>:
> Hello,

Hi,
> VersionInfo32_QueryValue seeks the requested element in a resource,
> and the check after that (which i patched) asks if this succeeded, and
> if it succeeded if it is text.
> Now there are some rare case where it succeeds, and where it is text
> but the length is actually 0.
> The data at this point should not matter, but because the check did
> not take into account the length returned, and blindly fed the data to
> WideCharToMultiByte (which uses some pointer trickery that in this
> very specific case is wrong to calculate the length), the result is
> VerQueryValueA telling you that it just found you x bytes of data,
> where it should have reported 0. (x depending on the data encountered
> at the current read position, usually 0 so usually it accidentally
> gives the correct value back.

The question is whether 0-length strings (i.e. not even the NULL
terminator) can happen or not. It's not clear to me with a quick look
at the tests.
So, while I'm not AJ, I think a test would make this more convincing.
> The 'dum' i introduced is purely to make sure the function doesnt trip
> if someone gives it a NULL pointer.
> regards,
>
> On Fri, Feb 13, 2015 at 11:27 AM, Stefan Dösinger
> <stefandoesinger(a)gmail.com> wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > Am 2015-02-12 um 20:15 schrieb Mark Jansen:
> > > LPWSTR lpSubBlockW;
> > > +        UINT dum = 0;
> > > +        if (!puLen)
> > > +            puLen = &dum;
> > >
> > > len  = MultiByteToWideChar(CP_ACP, 0, lpSubBlock, -1, NULL, 0);
> > >          lpSubBlockW = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> > > @@ -965,7 +968,7 @@ BOOL WINAPI VerQueryValueA( LPCVOID pBlock, LPCSTR lpSubBlock,
> > >
> > > HeapFree(GetProcessHeap(), 0, lpSubBlockW);
> > >
> > > -        if (ret && isText)
> > > +        if (ret && isText && *puLen)
> >
> > I am afraid I have no idea what this patch is doing. I am not even sure
> > I understand the original function properly :-( . VerQueryValueA seems
> > like an A to W wrapper for VersionInfo32_QueryValue, but with the quirky
> > behavior that it appends the ascii string after the unicode string.
> >
> > Can you elaborate a bit on what exactly goes wrong? What are the input
> > values, and what does VersionInfo32_QueryValue return?
> >
> > -----BEGIN PGP SIGNATURE-----
> > Version: GnuPG v2
> >
> > iQIcBAEBAgAGBQJU3dGbAAoJEN0/YqbEcdMwpwEP+wabxLJtbw7O7enIEu21l8ls
> > m6vxZr1PXP5RnTmOs4nk/QmWT4Oa68qY2EBdArZ79WwyVULLI7BjNJ1aLS67dw/Z
> > l6Dk9fh8orxoD4HznH0o7PxYrWNrQ1nJ2jf39k+TO4DbSHMZB/eGXnGUZxVFwKVf
> > H7yNxmoSISC0aWgQP0/QZF+AhtsJ5TLIWtNyJULpmTnZZGu9z94Ukftd9gDL+tZU
> > ezFsqxdDhpy0nirb4ky/Yq77jXQtsCmz/pCNi7ntYuQqY5fEmtJQMAtpfOQhdG5h
> > XP1Py7qFTu/RgqnBsO1Z3INRyKk7NS4BsPvULcLsk4WbDvoTGEqa1WwHwqHY4Dng
> > IDHagE938Xkc8CPiHGzqJG6WDdPPkFiRzqM+2HH87ihJ2uAaVZcK/V46lGiJCx8q
> > jDY+DlcDyZRmw28+fjwzOjPQHZx/jH2X3wLIpkliM5Ja03Ocb27Aaud9DEQ6H0vw
> > 0VbD2Db6oOrPBv3oyDmZhI+NTPgvsUoaH6sNE+3cIEwBWpCKkOJMzoDZ6EiqWVqj
> > jvJklcVcOn7nfUBH0YCfnG5szQokMnfcnZaOZxzkjWnPvwJbfC36eLQyESkbtbKf
> > Y2Z+mvbWslkhdeR0jKAmQNwfDuFcfvZflinem3U9xJAqBKEmicLeqJ0dtwUIJyC7
> > TnJMMCP14EDaX0DZGHw3
> > =94Ql
> > -----END PGP SIGNATURE-----

version: Fix wrong length reported when selecting a block. (resend)

[Mark Jansen]

Hello,

sorry for the late reply.
attached is a patch that demonstrates windows handles this fine, and
wine does not.

https://testbot.winehq.org/JobDetails.pl?Key=11597




On Mon, Feb 16, 2015 at 5:05 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
> 2015-02-13 20:41 GMT+01:00 Mark Jansen <learn0more+wine(a)gmail.com>:
> > Hello,
>
> Hi,
> > VersionInfo32_QueryValue seeks the requested element in a resource,
> > and the check after that (which i patched) asks if this succeeded, and
> > if it succeeded if it is text.
> > Now there are some rare case where it succeeds, and where it is text
> > but the length is actually 0.
> > The data at this point should not matter, but because the check did
> > not take into account the length returned, and blindly fed the data to
> > WideCharToMultiByte (which uses some pointer trickery that in this
> > very specific case is wrong to calculate the length), the result is
> > VerQueryValueA telling you that it just found you x bytes of data,
> > where it should have reported 0. (x depending on the data encountered
> > at the current read position, usually 0 so usually it accidentally
> > gives the correct value back.
>
> The question is whether 0-length strings (i.e. not even the NULL
> terminator) can happen or not. It's not clear to me with a quick look
> at the tests.
> So, while I'm not AJ, I think a test would make this more convincing.
> > The 'dum' i introduced is purely to make sure the function doesnt trip
> > if someone gives it a NULL pointer.
> > regards,
> >
> > On Fri, Feb 13, 2015 at 11:27 AM, Stefan Dösinger
> > <stefandoesinger(a)gmail.com> wrote:
> > > -----BEGIN PGP SIGNED MESSAGE-----
> > > Hash: SHA1
> > >
> > > Am 2015-02-12 um 20:15 schrieb Mark Jansen:
> > > > LPWSTR lpSubBlockW;
> > > > +        UINT dum = 0;
> > > > +        if (!puLen)
> > > > +            puLen = &dum;
> > > >
> > > > len  = MultiByteToWideChar(CP_ACP, 0, lpSubBlock, -1, NULL, 0);
> > > >          lpSubBlockW = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> > > > @@ -965,7 +968,7 @@ BOOL WINAPI VerQueryValueA( LPCVOID pBlock, LPCSTR lpSubBlock,
> > > >
> > > > HeapFree(GetProcessHeap(), 0, lpSubBlockW);
> > > >
> > > > -        if (ret && isText)
> > > > +        if (ret && isText && *puLen)
> > >
> > > I am afraid I have no idea what this patch is doing. I am not even sure
> > > I understand the original function properly :-( . VerQueryValueA seems
> > > like an A to W wrapper for VersionInfo32_QueryValue, but with the quirky
> > > behavior that it appends the ascii string after the unicode string.
> > >
> > > Can you elaborate a bit on what exactly goes wrong? What are the input
> > > values, and what does VersionInfo32_QueryValue return?
> > >
> > > -----BEGIN PGP SIGNATURE-----
> > > Version: GnuPG v2
> > >
> > > iQIcBAEBAgAGBQJU3dGbAAoJEN0/YqbEcdMwpwEP+wabxLJtbw7O7enIEu21l8ls
> > > m6vxZr1PXP5RnTmOs4nk/QmWT4Oa68qY2EBdArZ79WwyVULLI7BjNJ1aLS67dw/Z
> > > l6Dk9fh8orxoD4HznH0o7PxYrWNrQ1nJ2jf39k+TO4DbSHMZB/eGXnGUZxVFwKVf
> > > H7yNxmoSISC0aWgQP0/QZF+AhtsJ5TLIWtNyJULpmTnZZGu9z94Ukftd9gDL+tZU
> > > ezFsqxdDhpy0nirb4ky/Yq77jXQtsCmz/pCNi7ntYuQqY5fEmtJQMAtpfOQhdG5h
> > > XP1Py7qFTu/RgqnBsO1Z3INRyKk7NS4BsPvULcLsk4WbDvoTGEqa1WwHwqHY4Dng
> > > IDHagE938Xkc8CPiHGzqJG6WDdPPkFiRzqM+2HH87ihJ2uAaVZcK/V46lGiJCx8q
> > > jDY+DlcDyZRmw28+fjwzOjPQHZx/jH2X3wLIpkliM5Ja03Ocb27Aaud9DEQ6H0vw
> > > 0VbD2Db6oOrPBv3oyDmZhI+NTPgvsUoaH6sNE+3cIEwBWpCKkOJMzoDZ6EiqWVqj
> > > jvJklcVcOn7nfUBH0YCfnG5szQokMnfcnZaOZxzkjWnPvwJbfC36eLQyESkbtbKf
> > > Y2Z+mvbWslkhdeR0jKAmQNwfDuFcfvZflinem3U9xJAqBKEmicLeqJ0dtwUIJyC7
> > > TnJMMCP14EDaX0DZGHw3
> > > =94Ql
> > > -----END PGP SIGNATURE-----

version: Fix wrong length reported when selecting a block. (resend)

[Matteo Bruni]

2015-02-17 23:08 GMT+01:00 Mark Jansen <learn0more+wine(a)gmail.com>:
> Hello,
>
> sorry for the late reply.
> attached is a patch that demonstrates windows handles this fine, and
> wine does not.
>
> https://testbot.winehq.org/JobDetails.pl?Key=11597

Very nice, make sure to send the test to wine-patches too.
While you're at it, please avoid the Hungarian notation (i.e. replace
"szPreparedBuffer" with "buffer" or something) and declare the array
as static const. Looking good to me otherwise.