[PATCH 1/6] reg: Sanitize key paths in main

[Jonathan Vollebregt]

On 10/29/2014 04:38 PM, Stefan Dösinger wrote:
> Other than that you're still writing error messages from inside helper functions. Please try to return useful error codes and write the error messages from main() or the top level reg_*() functions. This should be a separate patch though, and it may mean adding some error case tests to advapi32.
>
> The thinking behind this is: There are plenty of reasons why Reg* functions can fail, not just that the key does not exist, e.g. permission denied, maybe others. Thus you will need some code that writes error messages that correspond to the Reg*Key return values anyway.
>
> There will always be errors that are specific to reg.exe though. E.g. it's not possible to pass a negative DWORD to RegSetValue.

How would I add reg.exe specific errors? Just make them ints over the 
16k used by system error codes?

In any case implementing this would require a big rewrite of most of the 
patches in the series, so I'll probably make a patch for the end of this 
series.

>> +static BOOL sane_path(const WCHAR *key)
>> +{
>> +    int i = strlenW(key);
>> +
>> +    if (i < 3 || (key[i - 1] == '\\' && key[i - 2] == '\\'))
>> +    {
>> +        reg_message(STRING_INVALID_KEY);
>> +        return 0;
> I still don't see why you need this check. If you remove it path_get_key should produce the same error message because it can't find a root node "\FOOBAR".

This checks for backslashes at the end of the key not the start - tests 
show more than one should produce an error.

>> -        reg_type = get_regtype(type);
>> +        if (!type)
>> +            reg_type = REG_SZ;
>> +        else
>> +            reg_type = wchar_get_type(type);
> Why not keep the !type check inside get_regtype?

It's possible for something calling this to want a different default, so 
I decided to let the caller handle it.

> Patch 4:
>> +            i = (strlenW(input) + 1) * sizeof(WCHAR);
>> +            output = HeapAlloc(GetProcessHeap(), 0, i);
>> +            lstrcpyW((WCHAR *) output, input);
> Since you know the length, you can use memcpy. You could also think about a strdupW_heap() helper function if you need this in multiple places, but then reusing the length is more difficult.

Does this mean I'm allowed to use malloc too?

> Patch 5:
>> +        if (!force && RegQueryValueW(key, value_name, NULL, NULL) == ERROR_SUCCESS)
>>           {
>> -            if (RegQueryValueW(subkey,value_name,NULL,NULL)==ERROR_SUCCESS)
>> -            {
>> -                /* FIXME:  Prompt for overwrite */
>> -            }
>> +            /* FIXME:  Prompt for overwrite */
>>           }
> Afaics this was and still is dead code. Unless RegQueryValue has a side effect either just keep the comment or write an actual FIXME.

The idea is presumably only to prompt for overwrite if the value already 
exists. That's what the RegQueryValue is for. Presumably a y/n prompt 
should go here but I have my hands full with the rest of it.

> If the subkey does not exist, and invalid data is passed, you're creating the subkey without adding the value and returning an error. This is likely not the correct behavior.

Should it prompt for new key creation as well as overwriting? I'm not 
sure what the native behavior is