git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v6 1/1] mingw: give more details about unsafe directory's ownership

From
Johannes Sixt <j6t@kdbg.org>
Date
Jan 7, 2024, 20:02 UTC
Message-ID
<de9cf40a-1ad6-45fb-8b70-8b0c71a3bfbb@kdbg.org>
In-Reply-To
<20240106112917.1870-2-soekkle@freenet.de>
Am 06.01.24 um 12:29 schrieb Sören Krecker:
Show 26 quoted lines
> Add domain/username in error message, if owner sid of repository and
> user sid are not equal on windows systems.
> 
> Old error message:
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
> 	'S-1-5-21-571067702-4104414259-3379520149-500'
> but the current user is:
> 	'S-1-5-21-571067702-4104414259-3379520149-1001'
> To add an exception for this directory, call:
> 
> 	git config --global --add safe.directory C:/Users/test/source/repos/git
> '''
> 
> New error message:
> '''
> fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git'
> 'C:/Users/test/source/repos/git' is owned by:
>         'DESKTOP-L78JVA6/Administrator' (S-1-5-21-571067702-4104414259-3379520149-500)
> but the current user is:
>         'DESKTOP-L78JVA6/test' (S-1-5-21-571067702-4104414259-3379520149-1001)
> To add an exception for this directory, call:
> 
>         git config --global --add safe.directory C:/Users/test/source/repos/git
> '''

I am not a fan of putting everything and the kitchen sink inside quotes. In particular, the single-quotes around the user names are unnecessary, IMO. Would you mind dropping them? (I do not mean to remove the quotes around the path names because you do not touch this part of the code.)

Show 24 quoted lines
> 
> Signed-off-by: Sören Krecker <soekkle@freenet.de>
> ---
>  compat/mingw.c | 64 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 51 insertions(+), 13 deletions(-)
> 
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 42053c1f65..6240387205 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2684,6 +2684,26 @@ static PSID get_current_user_sid(void)
>  	return result;
>  }
>  
> +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*/

This comment line doesn't follow our style. Please either fix that (add blanks after /* and before */ or (my preference) remove it altogether; the code is clear without it.

> +	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)
We prefer to write this condition as
	if (!translate_sid_to_user)
Show 15 quoted lines
> +		FREE_AND_NULL(*str);
> +	else
> +		(*str)[len_domain] = '/';
> +	return translate_sid_to_user;
> +}
> +
>  static int acls_supported(const char *path)
>  {
>  	size_t offset = offset_1st_component(path);
> @@ -2765,27 +2785,45 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report)
>  			strbuf_addf(report, "'%s' is on a file system that does "
>  				    "not record ownership\n", path);
>  		} else if (report) {
> -			LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL;
> +			LPSTR str1, str2, str3, str4, to_free1 = NULL, to_free3 = NULL, to_local_free2=NULL, to_local_free4=NULL;

This line grew a bit long now. Maybe break it to have lines not exceed 80 characters? While you do so, please insert blanks around = signs.

Show 53 quoted lines
>  
> -			if (ConvertSidToStringSidA(sid, &str1))
> +			if (user_sid_to_user_name(sid, &str1))
>  				to_free1 = str1;
>  			else
>  				str1 = "(inconvertible)";
> -
> -			if (!current_user_sid)
> -				str2 = "(none)";
> -			else if (!IsValidSid(current_user_sid))
> -				str2 = "(invalid)";
> -			else if (ConvertSidToStringSidA(current_user_sid, &str2))
> -				to_free2 = str2;
> +			if (ConvertSidToStringSidA(sid, &str2))
> +				to_local_free2 = str2;
>  			else
>  				str2 = "(inconvertible)";
> +
> +			if (!current_user_sid) {
> +				str3 = "(none)";
> +				str4 = "(none)";
> +			}
> +			else if (!IsValidSid(current_user_sid)) {
> +				str3 = "(invalid)";
> +				str4 = "(invalid)";
> +			} else {
> +				if (user_sid_to_user_name(current_user_sid,
> +							  &str3))
> +					to_free3 = str3;
> +				else
> +					str3 = "(inconvertible)";
> +				if (ConvertSidToStringSidA(current_user_sid,
> +							   &str4))
> +					to_local_free4 = str4;
> +				else
> +					str4 = "(inconvertible)";
> +			}
>  			strbuf_addf(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);
> +				    "\t'%s' (%s)\nbut the current user is:\n"
> +				    "\t'%s' (%s)\n",
> +				    path, str1, str2, str3, str4);
> +			free(to_free1);
> +			LocalFree(to_local_free2);
> +			free(to_free3);
> +			LocalFree(to_local_free4);
>  		}
>  	}
>  

Aside from this, the patch works well for me. It is a real usability improvement. Thank you for working on it.

-- Hannes
Previous: Sören KreckerNext: Sören Krecker
Message 19 of 28 in “Replace SID with domain/username on Windows”
  1. 0/1 Replace SID with domain/username on WindowsSören Krecker, Dec 29, 2023
  2. 1/1 Replace SID with domain/usernameSören Krecker, Dec 29, 2023
  3. Junio C HamanoJan 2, 2024
  4. Junio C HamanoJan 2, 2024
  5. Eric SunshineDec 31, 2023
  6. 0/1 Replace SID with domain/username on WindowsSören Krecker, Dec 31, 2023
  7. 1/1 Replace SID with domain/usernameSören Krecker, Dec 31, 2023
  8. Junio C HamanoJan 2, 2024
  9. 0/1 Replace SID with domain/username on WindowsSören Krecker, Jan 2, 2024
  10. 1/1 Replace SID with domain/usernameSören Krecker, Jan 2, 2024
  11. Junio C HamanoJan 3, 2024
  12. Matthias AßhauerJan 3, 2024
  13. Junio C HamanoJan 3, 2024
  14. 0/1 Replace SID with domain/username on WindowsSören Krecker, Jan 4, 2024
  15. 1/1 Adds domain/username to error messageSören Krecker, Jan 4, 2024
  16. Junio C HamanoJan 4, 2024
  17. 0/1 mingw: give more details about unsafe directory's ownershipSören Krecker, Jan 6, 2024
  18. 1/1 mingw: give more details about unsafe directory's ownershipSören Krecker, Jan 6, 2024
  19. Johannes SixtJan 7, 2024
  20. 0/1 mingw: give more details about unsafe directory'sSören Krecker, Jan 8, 2024
  21. 1/1 mingw: give more details about unsafe directory's ownershipSören Krecker, Jan 8, 2024
  22. Junio C HamanoJan 8, 2024
  23. Johannes SixtJan 9, 2024
  24. Junio C HamanoJan 9, 2024
  25. Johannes SixtJan 9, 2024
  26. Junio C HamanoJan 9, 2024
  27. Dragan SimicJan 8, 2024
  28. Eric SunshineDec 31, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.