Hi Aric,
it strikes me that this could be broken up a bit further. This'd make it rather easier to review. For example,
--- a/dlls/advapi32/advapi32_misc.h +++ b/dlls/advapi32/advapi32_misc.h @@ -24,4 +24,15 @@ const char * debugstr_sid(PSID sid); BOOL ADVAPI_IsLocalComputer(LPCWSTR ServerName); BOOL ADVAPI_GetComputerSid(PSID sid);
+BOOL lookup_local_wellknown_name(LPCWSTR lpAccountName, DWORD ccAccountName, + PSID Sid, LPDWORD cbSid, + LPWSTR ReferencedDomainName, + LPDWORD cchReferencedDomainName, + PSID_NAME_USE peUse, BOOL *ret); +BOOL lookup_local_user_name(LPCWSTR lpAccountName, DWORD ccAccountName, + PSID Sid, LPDWORD cbSid, + LPWSTR ReferencedDomainName, + LPDWORD cchReferencedDomainName, + PSID_NAME_USE peUse, BOOL *ret);
In the initial patch, these aren't referenced outside of security.c, so leaving them static there would make the changes smaller. A subsequent patch that does reference them from lsa.c could make them non-static.
+static inline WCHAR *strnrchrW(const WCHAR *str, WCHAR ch, DWORD *len)
It's not clear to me what this function does from its name, and you didn't document it. There's no corresponding strnrchr libc function, so that doesn't help either.
It appears from reading it that you made a function like strrchrW that returns the location of the found ch in str, like strrchrW does, but also sets *len to the position at which it's found?
If that's correct, you could accomplish the same thing by calling strrchrW and doing pointer arithmetic with its return value.
These suggestions notwithstanding, it seems like introducing this helper function is one change, and deserving of its own patch.
+ +/* + * Helper function for LookupAccountNameW and LookupNames2 in lsa.c
The second part of that comment, that it's a helper for LookupNames2, isn't correct in this first patch. It becomes true later, but the comment should be changed then, not in the first patch. (Minor nit: mentioning the file LookupNames2 is in seems superfluous.)
+ * returns TRUE if the AccountName is handled (even if with an error). + * returns FALSE if not. + */ + +BOOL lookup_local_wellknown_name(LPCWSTR lpAccountName, DWORD ccAccountName, + PSID Sid, LPDWORD cbSid, + LPWSTR ReferencedDomainName, + LPDWORD cchReferencedDomainName, + PSID_NAME_USE peUse, BOOL *ret)
The use of two BOOLs is confusing to me. For one thing, it makes the patch larger:
- ret = CreateWellKnownSid(ACCOUNT_SIDS[i].type, NULL, pSid, &sidLen); + *ret = CreateWellKnownSid(ACCOUNT_SIDS[i].type, NULL, pSid, &sidLen);
This change could be avoided if you returned the old meaning of ret, and used the BOOL * parameter to indicate whether the AccountName is handled.
Finally, I'd suggest that breaking out looking up well known names could be separate from breaking out looking up local names.
Hope that helps, --Juan