[1/1] Replace SID with domain/username

Commit Message

Sören Krecker Dec. 28, 2023, 1:28 p.m. UTC

From: soekkle <[e-mail]>

Replace SID with domain/username in erromessage, if owner of repository
and user are not equal on windows systems.

Signed-off-by: Sören Krecker <[e-mail]>
---
 compat/mingw.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

Comments

Eric Sunshine Dec. 28, 2023, 7:27 p.m. UTC | #1

On Thu, Dec 28, 2023 at 8:29 AM Sören Krecker <[e-mail]> wrote:
> From: soekkle <[e-mail]>
>
> Replace SID with domain/username in erromessage, if owner of repository
> and user are not equal on windows systems.
>
> Signed-off-by: Sören Krecker <[e-mail]>
> ---

I don't do Windows (anymore), thus I'm not qualified to comment on the
substance of this patch, so I'll just make some general, hopefully
helpful, observations.

Typo: "erromessage" should be "error message"

Your name in the "From:" header and Signed-off-by: should be the same.

Perhaps Widows folks can understand the purpose of this patch without
further explanation, but for other readers, it's not clear what
problem the patch is trying to solve. The commit message is a good
place to explain _why_ this change is desirable.

> diff --git a/compat/mingw.c b/compat/mingw.c
> @@ -2684,6 +2684,25 @@ static PSID get_current_user_sid(void)
> +BOOL user_sid_to_string(PSID sid, LPSTR* str)

In this codebase, '*' sticks to the variable name, not the type, so:

    BOOL user_sid_to_string(PSID sid, LPSTR *str)

> +{
> +       SID_NAME_USE peUse;
> +       DWORD lenName = { 0 }, lenDomain = { 0 };

Looking through compat/mingw.c, it appears that (as with the rest of
the project), variable names tend to use underscores rather than
camel-case, so for consistency these might be better expressed as
"pe_use" (whatever that means), "name_len", and "domain_len".

I was curious about the `{ 0 }` initializer. It seems we have a mix of
both `{0}` and `{ 0 }` in the codebase, so what you have here is
likely fine.

> +       LookupAccountSidA(NULL, sid, NULL, &lenName, NULL,
> +                                       &lenDomain, &peUse); // returns only FALSE, because the string pointers are NULL

As with the rest of the project, compat/mingw.c still shuns "//"
comments. Use /*...*/ comments instead.

> +       ALLOC_ARRAY((*str), (size_t)lenDomain + (size_t)lenName); // Alloc neded Space of the strings

Type: "neded" -> "needed"

(and "Space" -> "space")

> +       BOOL retVal = LookupAccountSidA(NULL, sid, (*str) + lenDomain, &lenName,
> +                                      *str,
> +                                       &lenDomain, &peUse);
> +       *(*str + lenDomain) = '/';
> +       if (retVal == FALSE)
> +       {
> +               free(*str);
> +               *str = NULL;

The FREE_AND_NULL() macro from git-compat-util.h is a good companion
to the ALLOC_ARRAY() macro used above, so freeing and nullifying could
be done in one line:

    FREE_AND_NULL(*str);

> +       }
> +       return retVal;
> +}

Perhaps a variable name such as `ok` would convey more to the reader
than the generic `retVal`?

Patch

diff --git a/compat/mingw.c b/compat/mingw.c
index 42053c1f65..7318f0e20e 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2684,6 +2684,25 @@  static PSID get_current_user_sid(void)
 	return result;
 }
 
+BOOL user_sid_to_string(PSID sid, LPSTR* str)
+{
+	SID_NAME_USE peUse;
+	DWORD lenName = { 0 }, lenDomain = { 0 };
+	LookupAccountSidA(NULL, sid, NULL, &lenName, NULL,
+					&lenDomain, &peUse); // returns only FALSE, because the string pointers are NULL
+	ALLOC_ARRAY((*str), (size_t)lenDomain + (size_t)lenName); // Alloc neded Space of the strings
+	BOOL retVal = LookupAccountSidA(NULL, sid, (*str) + lenDomain, &lenName,
+				       *str,
+					&lenDomain, &peUse);
+	*(*str + lenDomain) = '/';
+	if (retVal == FALSE)
+	{
+		free(*str);
+		*str = NULL;
+	}
+	return retVal;
+}
+
 static int acls_supported(const char *path)
 {
 	size_t offset = offset_1st_component(path);
@@ -2767,7 +2786,7 @@  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_string(sid, &str1))
 				to_free1 = str1;
 			else
 				str1 = "(inconvertible)";
@@ -2776,7 +2795,7 @@  int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 				str2 = "(none)";
 			else if (!IsValidSid(current_user_sid))
 				str2 = "(invalid)";
-			else if (ConvertSidToStringSidA(current_user_sid, &str2))
+			else if (user_sid_to_string(current_user_sid, &str2))
 				to_free2 = str2;
 			else
 				str2 = "(inconvertible)";
@@ -2784,8 +2803,8 @@  int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
 				    "'%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);
 		}
 	}