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

Re: [PATCH] repack.c: chmod +w before rename()

From
brian m. carlson <sandals@crustytoothpaste.net>
Date
Jan 24, 2014, 21:40 UTC
Message-ID
<20140124214023.GB58260@vauxhall.crustytoothpaste.net>
In-Reply-To
<201401242205.16313.tboegi@web.de>
On Fri, Jan 24, 2014 at 10:05:14PM +0100, Torsten Bögershausen wrote:
Show 43 quoted lines
> commit a1bbc6c0 "repack: rewrite the shell script in C" introduced
> a possible regression, when a Git repo is located on a Windows network share.
> 
> When git gc is called on an already packed repository, it could fail like this:
> "fatal: renaming '.git/objects/pack/.tmp-xyz.pack' failed: Permission denied"
> 
> Temporary *.pack and *.idx files remain in .git/objects/pack/
> 
> In a1bbc6c0 the rename() function replaced the "mv -f" shell command.
> 
> Obviously the -f option can also override a read-only file but
> rename() can not on a network share.
> 
> Make the C-code to work similar to the shell script, by making the
> files writable before calling rename().
> 
> Another solution could be to do the "chmod +x" in mingw_rename().
> This may be done in another commit, because
> a) It improves git gc only when Git for Windows is used
>    on the client machine
> b) Windows refuses to delete a file when the file is read-only.
>    So setting a file to read-only under Windows is a way for a user
>    to protect it from being deleted.
>    Changing the behaviour of rename() globally may not be what we want.
> 
> Reported-by: Jochen Haag <zwanzig12@googlemail.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  builtin/repack.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/builtin/repack.c b/builtin/repack.c
> index ba66c6e..033b4c2 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -324,6 +324,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  				statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
>  				chmod(fname_old, statbuffer.st_mode);
>  			}
> +			if (!stat(fname, &statbuffer)) {
> +				statbuffer.st_mode |= (S_IWUSR | S_IWGRP | S_IWOTH);
> +				chmod(fname, statbuffer.st_mode);
> +			}

Are we sure that the file in question can never be a symlink? Because if it is, we'll end up changing the permissions on the wrong file. In general, I'm wary of changing permissions on a file to suit Windows's rename because of the symlink issue and the security issues that can result. Hard links probably have the same issue.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187
Previous: Torsten BögershausenNext: Johannes Schindelin
Message 2 of 7 in “repack.c: chmod +w before rename()”
  1. repack.c: chmod +w before rename()Torsten Bögershausen, Jan 24, 2014
  2. brian m. carlsonJan 24, 2014
  3. Johannes SchindelinJan 24, 2014
  4. brian m. carlsonJan 24, 2014
  5. Mike HommeyJan 24, 2014
  6. Junio C HamanoJan 24, 2014
  7. Junio C HamanoJan 24, 2014

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.