Re: [PATCH v4 5/8] reg.exe: Add wchar/raw data conversion functions

[Stefan Dösinger]

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

This code could use more tests, especially for REG_SZ_MULTI and REG_BINARY.

Am 2014-10-09 10:55, schrieb Jonathan Vollebregt:
> +static WCHAR *data_get_wchar(const BYTE *data, const DWORD size, const DWORD type)
> This function is not used until the next patch. From a design point of view it unfortunate that you allocate a new string for the REG_SZ and REG_SZ_MULTI case and essentially just do a memcpy and free the old string. As long as you just need this function to print the values it would be better to add a function that prints directly.

Keep reg export in mind though. It may need the function like this, or maybe a printing function that is passed a file descriptor.
> +            do
> +            {
> +                lstrcpyW(output+i, &input[i]);
> +
> +                i += strlenW(&input[i]) + 1;
> +
> +                if (input[i] != 0)
> +                    output[i - 1] = ',';
> +            } while (input[i]);
> If I understand this correctly this replaces 0 WCHARs with ',' . There should be nicer ways to do this.
> +static BYTE *wchar_get_data(const WCHAR *input,     const DWORD type,
> +                            const WCHAR seperator,  DWORD *size_out)
> ...
> +        case REG_SZ:
> +        case REG_EXPAND_SZ:
> +        {
> +            i = (strlenW(input) + 1) * sizeof(WCHAR);
> +            output = HeapAlloc(GetProcessHeap(), 0, i);
> +            lstrcpyW((WCHAR *) output, input);
> +            *size_out = i;
> +            return output;
> +        }
> This also has unfortunate copying behavior. Maybe there is a nicer way to solve this.
> +            for (i = 0, p = 0; i <= strlenW(input); i++, p++)
> +            {
> +                if (input[i] == seperator)
> Please watch the spelling of separator. This confused me a bit in my code search :-) .
> +                    temp[p] = 0;
> +                else if (seperator == 0 && input[i] == '\\' && input[i + 1] == '0')
> How does a 0 separator work? The command line interpreter would fail to parse such a string. msdn claims that the default separator is '\0', but maybe they mean a backslash ('\\') like your code uses. If the default separator is a backslash it would be nicer to handle this default in the code that parses the command line.
> +                {
> +                    temp[p] = 0;
> +                    i++;
> +                }
> I don't understand what's special about a 0 or \ separator as far as this loop is concerned.
> +            if (p % 2)
> if (p & 1) is nicer to check for odd / even (i.e., the lowest bit). The compiler should be able to to this optimization as well.
> +            p /= 2;
> p >>= 1 . Or you could consider incrementing i by 2 in the for loop below.
> +            for (i = 0; i < p; i++)
> +                temp[i] = (temp[i * 2] << 4) | temp[i * 2 + 1];

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJUOpgoAAoJEN0/YqbEcdMwyLYP+QFSbpZLsy/Au6uKLQNfcRqS
MnHxgrB1wq+RxfDbQ6UdzT6gIVKhEII0c//gZ32H5Fr/+zIofaa38XmtH6Fp8MMg
p1s+ubFeZ29vMaNER28zwK/QM6Allwo+JLjqLSM6W2cgrfe1kU/iPWGC97nJktEj
cG4wsAdcgYCHvkOH/06ZIC/zSbWyG0yZJSgKfLqfgEbuD/TAmmkXh0TVMm9Znp88
J6mLhcYSbfTK2oHzU9nXETTa5oFR/VfHQHZ7asa+q14kftl24Y5JCs1VD6BK/G4p
TjD5jQhIN9L5FHKj79n+9y5eeYjrIiMhdi81ey3Bm8it03ZPUN3aAjXf6CD6UmBu
wxHZwB8L1PALJsDBK+E7a8QgeteWxMjX07m2FdfFNewj4IFq4eoASdeINd8Zd1rt
TpWl2/jpKz4PecRvGeNKOEgqx+mFcSRWiXErytWtEmJzz+aPEjdjFXesf6aWFB/l
+ZZxc202He9JXacPFvmCuL0ltWStJqdSZRZg/FkIgqXZkWfj1bwxDYpu+7Jv6243
KrQrMUGna1VXGM6tAfYph0BLtZnksTwFS6MEoPzirf2Yqe3uuQzecQqhyaStaESJ
5HS+6NTEZT2LhvKy3mui970k2THpmQFb7j4f+xdmKHEj6qdsoH+SHouJgkig/KLk
x28pWN1J80RCE92BQdYV
=tqtd
-----END PGP SIGNATURE-----

[PATCH v4 5/8] reg.exe: Add wchar/raw data conversion functions

[Jonathan Vollebregt]

Clearing some stuff up here

On 10/12/2014 05:03 PM, Stefan Dösinger wrote:
> From a design point of view it unfortunate that you allocate a new string for the REG_SZ and REG_SZ_MULTI case and essentially just do a memcpy and free the old string.

I don't free the old string. Because the function handles more than one 
type of data the caller has to free the data when it's done - if I don't 
copy the string it will end up freeing one of it's own parameters.

Now that I think of it I'm not sure what will happen then, but 
presumably you'll get a segfault since parameters should be on the stack.
> If I understand this correctly this replaces 0 WCHARs with ',' . There should be nicer ways to do this.

Sorry, this seems confusing without comments. The REG_MULTI_SZ stores 
multiple strings in the registry: nil separated with a double nil to end 
the array.

When reading from the registry simply replacing the nil's with ',' seems 
the simplest way to do it, and that's exactly wine's regedit's 
behaviour. I'm not sure what the native behaviour is supposed to be though.
> How does a 0 separator work? The command line interpreter would fail to parse such a string. msdn claims that the default separator is '\0', but maybe they mean a backslash ('\\') like your code uses. If the default separator is a backslash it would be nicer to handle this default in the code that parses the command line.

If the separator has been left out of the commandline, the separator 
variable here will be 0. By default reg (From everything I've read) 
interprets "\\0" as the separator - that is it takes an escaped nil as 
it's input to split the strings.

If you *do* supply a separator variable though it will split by that, so 
the following commands should have an equivalent effect:

reg add HKCU /v test /t REG_MULTI_SZ /d "string\\0morestring"
reg add HKCU /v test /t REG_MULTI_SZ /s "#" /d "string#morestring"

[PATCH v4 5/8] reg.exe: Add wchar/raw data conversion functions

[Stefan Dösinger]

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

Am 2014-10-12 19:47, schrieb Jonathan Vollebregt:
> I don't free the old string. Because the function handles more than
> one type of data the caller has to free the data when it's done -
> if I don't copy the string it will end up freeing one of it's own
> parameters.
> I used bad terminology here. What I meant was that wchar_get_data
> could operate on the input memory instead of copying it first since
> the original data is not needed after the call.

Of course further features may require that the input remains
untouched. Is this the case?

There's another ugliness though: If the input string is "0", replacing
it with a 4-byte DWORD just works because it is a WCHAR that consists
of two two bytes elements. I don't know about other data types - they
may actually grow.
> Sorry, this seems confusing without comments. The REG_MULTI_SZ
> stores multiple strings in the registry: nil separated with a
> double nil to end the array.
>
> When reading from the registry simply replacing the nil's with ','
> seems the simplest way to do it, and that's exactly wine's
> regedit's behaviour. I'm not sure what the native behaviour is
> supposed to be though.
> As I mentioned tests would be a good idea :-) .

I understood what the function was doing after enough reading. What I
meant was that there was probably a nicer way to implement the same
behavior. E.g. if you do the character replace in the existing string
you could write this as

for (i = 0; inout[i] || input[i + 1]; i++)
{
    if (!inout[i])
        inout[i] = ',';
}

Or MakeMULTISZDisplayable() from regedit. MakeMULTISZDisplayable is
probably faster because it does only one comparison per character.

But if you cannot operate on the input data that idea is moot. You
could copy first and then run a second iteratation over the string,
but I think that results in worse performance than your code.
> If the separator has been left out of the commandline, the
> separator variable here will be 0. By default reg (From everything
> I've read) interprets "\\0" as the separator - that is it takes an
> escaped nil as it's input to split the strings.
>
> If you *do* supply a separator variable though it will split by
> that, so the following commands should have an equivalent effect:
>
> reg add HKCU /v test /t REG_MULTI_SZ /d "string\\0morestring" reg
> add HKCU /v test /t REG_MULTI_SZ /s "#" /d "string#morestring"
> You mean it uses a two-byte separator '\\','0', rather than the
> one-byte separator '\0'? In this case it would be best to make the
> code work on generic string separators. The default separator needs
> tests as well.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIbBAEBAgAGBQJUOs42AAoJEN0/YqbEcdMw7LwP9RJ8PddCWJpZqRATzimyWe3X
pQDhtf3rE696zA3w4lMXom++V5YJDFkJp3wmX7EMyH2+EKROFBVkhPV9C2dk+eFc
m8Wfrq5+HSOc7nb2uB6c7AqZMVHLbooSFIHsiX0lmmDDdPI1I60EUhdk243i6PeR
c4ItRWbqDAj2Fa/4b4dZ7K3O1OyBDpT0fChmFJabtFMFQPoOxZhgv8SJbMC0Uv0K
0HVN8vsizLkMycAYk7cf4P914b+u9uPSDWsllvfMmgh+ABo2pDd6b57N8abVABoC
y6fDLasFuPBbAxeJKjVbMvKRvuNAvkEHj4XmvxPoaniFBamsC7Mr13T9mPiyhXDR
yhbHbI5w3AZdCPRjbe2B25jtwthNriVQ/C+D9GGENvuw0qxj0WiAsPB9xllMSYYZ
E9OaDPEpcV0NmyCdDyV3jIiMupZ8uk2wqaHHCkWkJx2r4/V3iDYuAuOhQVZsjrei
JAuAShe3ZTkvoGTNDhL43eMq7Wxp5LhMIa03JQVuQCGFZgfrd3TotlAHVtfGYpSc
oMuxN8Uv2vx7s9eg32yvovWsjoJDcUlXaQkZVtS4j0ZXXM1Cu8LcBglzbJj8303+
XLBO2fxs3gV3THziOemCLD+IYyKuXOoqTEgpmPUuT5NSoPeFywJKiuOlqcMwikrW
P80DHs/eIFBWbJcLRLo=
=AGZl
-----END PGP SIGNATURE-----