Re: [PATCH V4 1/1] Replace SID with domain/username
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 3, 2024, 00:43 UTC
- Message-ID
- <xmqqa5pnckm4.fsf@gitster.g>
- In-Reply-To
- <20240102191514.2583-2-soekkle@freenet.de>
Sören Krecker <soekkle@freenet.de> writes:
> Replace SID with domain/username in error message, if owner of repository > and user are not equal on windows systems. Each user should have a unique > SID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers).
That paragraph your URL refers to does say that a SID that is used for an account will never be reused to identify a different account. But I am not sure if it means a user will never be assigned more than one SID (in other words, the reverse is not necessarily true).
The paragraph also mentions that a SID can identify a non-user entity like a computer account (as opposed to "a user account")---I do not know what its implications are in the context of this patch, though.
> This means that domain/username is not a loss of information.
This statement does not (grammatically) make sense, but more importantly, loss of information may not be a bad thing in this case. If more than one SIDs are given to a user account and processes working for that account, these different SIDs may be translated, by using LookupAccountSidA(), to the same string for a single user@domain, and it would be an operation that loses information in that sense.
But if what we *care* about is user@domain between the current process and the owner of the directory in question being the same (or not), then such a loss of information is a *good* thing.
So I dunno. Arguing what we care about (is that exact SID equality between the "owner of the directory" and the "user, which the current process is working on behalf of", or do we care about the equality of the "accounts"?) may be a better way to justify this change, if you ask me.
Show 16 quoted lines
> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str)
> +{
> + SID_NAME_USE pe_use;
> + DWORD len_user = 0, len_domain = 0;
> + BOOL translate_sid_to_user;
> +
> + /* returns only FALSE, because the string pointers are NULL*/
> + LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain,
> + &pe_use);
> + /*Alloc needed space of the strings*/
> + ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user);
> + translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user,
> + *str, &len_domain, &pe_use);
> + if (translate_sid_to_user == FALSE) {
> + FREE_AND_NULL(*str);
> + }Style: do not enclose a single-statement block inside {}.> + else > + (*str)[len_domain] = '/'; > + return translate_sid_to_user; > +}
Show 9 quoted lines
> @@ -2767,7 +2788,9 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
> } else if (report) {
> LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
>
> - if (ConvertSidToStringSidA(sid, &str1))
> + if (user_sid_to_user_name(sid, &str1))
> + to_free1 = str1;
> + else if (ConvertSidToStringSidA(sid, &str1))
> to_free1 = str1;Do these two helper functions return pointers pointing into the same kind of memory that you can free with the same function? That is ...
Show 8 quoted lines
> ... > "'%s' is owned by:\n" > "\t'%s'\nbut the current user is:\n" > "\t'%s'\n", path, str1, str2); > - LocalFree(to_free1); > - LocalFree(to_free2); > + free(to_free1); > + free(to_free2);
... the original code seems to say that the piece of memory we obtain from ConvertSidToStringSidA() must not be freed by calling free() but use something special called LocalFree(). I am assuing that your user_sid_to_user_name() returns a regular piece of memory that can be freed by calling regular free()? Do we need to keep track of where we got the memory from and use different function to free each variable, or something (again I do not do Windows so I'll defer all of these to Dscho, who is CC'ed this time).
Thanks and a happy new year.
> } > }