-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Am 2014-10-29 00:19, schrieb Jonathan Vollebregt:
RegOpenKey always returns an error if path starts with '\', which is why theres an increment on path right after the strchrW
Of course if path_get_key returns NULL we'll be passing in the equivalent of `(WCHAR *)2` so the NULL check prevents segfaults.
Right, my bad.
We could replace the return with the path increment so that RegOpenKey gets NULL as it's parameter like this:
It has one less branch (no early return), but still has the if, so I guess it is an improvement, although not as much as I hoped. I'll leave this up to you.
Another alternative idea, but I don't know how well this can work: path_get_key already looks at the '\' behind the root key. You could change this function to return a root key and a WCHAR that points to the rest of the path sans \. But don't bother too much about this, I think that's a fairly minor detail.
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.
Other things:
+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;
Please return TRUE or FALSE from a function that returns a BOOL type.
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".
The remote check ("\FOOBAR") is necessary though - it writes a different error message.
key_name = argvW[2];
if (!sane_path(argvW[2]))return 1;
Minor detail: Pass key_name to sane_path.
Patch 3:
if (strcmpiW(type_rels[i].name, type_name) == 0)return type_rels[i].type;
if (!strcmpiW(...)) is preferred over if (strcmpiW(...) == 0)
reg_type = get_regtype(type);
if (!type)reg_type = REG_SZ;elsereg_type = wchar_get_type(type);
Why not keep the !type check inside get_regtype?
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.
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.
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.