{"thread":{"id":"35728","subject":"[PATCH] repack.c: chmod +w before rename()","startedAt":"2014-01-24T21:05:14Z","lastAt":"2014-01-24T23:50:48Z","messageCount":7,"participants":["Torsten Bögershausen","brian m. carlson","Johannes Schindelin","Junio C Hamano","Mike Hommey"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"233722","messageId":"201401242205.16313.tboegi@web.de","threadId":"35728","inReplyTo":null,"subject":"[PATCH] repack.c: chmod +w before rename()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-01-24T21:05:14Z","receivedAt":"2014-01-24T21:05:14Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"commit a1bbc6c0 \"repack: rewrite the shell script in C\" introduced\na possible regression, when a Git repo is located on a Windows network share.\n\nWhen git gc is called on an already packed repository, it could fail like this:\n\"fatal: renaming '.git/objects/pack/.tmp-xyz.pack' failed: Permission denied\"\n\nTemporary *.pack and *.idx files remain in .git/objects/pack/\n\nIn a1bbc6c0 the rename() function replaced the \"mv -f\" shell command.\n\nObviously the -f option can also override a read-only file but\nrename() can not on a network share.\n\nMake the C-code to work similar to the shell script, by making the\nfiles writable before calling rename().\n\nAnother solution could be to do the \"chmod +x\" in mingw_rename().\nThis may be done in another commit, because\na) It improves git gc only when Git for Windows is used\n on the client machine\nb) Windows refuses to delete a file when the file is read-only.\n So setting a file to read-only under Windows is a way for a user\n to protect it from being deleted.\n Changing the behaviour of rename() globally may not be what we want.\n\nReported-by: Jochen Haag \nSigned-off-by: Torsten Bögershausen \n---\n builtin/repack.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex ba66c6e..033b4c2 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -324,6 +324,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n \t\t\t\tchmod(fname_old, statbuffer.st_mode);\n \t\t\t}\n+\t\t\tif (!stat(fname, &statbuffer)) {\n+\t\t\t\tstatbuffer.st_mode |= (S_IWUSR | S_IWGRP | S_IWOTH);\n+\t\t\t\tchmod(fname, statbuffer.st_mode);\n+\t\t\t}\n \t\t\tif (rename(fname_old, fname))\n \t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n \t\t\tfree(fname);\n-- \n1.9.rc0.143.g6fd479e\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/groups/opt_out.\n"},{"id":"233727","messageId":"20140124214023.GB58260@vauxhall.crustytoothpaste.net","threadId":"35728","inReplyTo":"201401242205.16313.tboegi@web.de","subject":"Re: [PATCH] repack.c: chmod +w before rename()","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-01-24T21:40:23Z","receivedAt":"2014-01-24T21:40:23Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Jan 24, 2014 at 10:05:14PM +0100, Torsten Bögershausen wrote:\n> commit a1bbc6c0 \"repack: rewrite the shell script in C\" introduced\n> a possible regression, when a Git repo is located on a Windows network share.\n> \n> When git gc is called on an already packed repository, it could fail like this:\n> \"fatal: renaming '.git/objects/pack/.tmp-xyz.pack' failed: Permission denied\"\n> \n> Temporary *.pack and *.idx files remain in .git/objects/pack/\n> \n> In a1bbc6c0 the rename() function replaced the \"mv -f\" shell command.\n> \n> Obviously the -f option can also override a read-only file but\n> rename() can not on a network share.\n> \n> Make the C-code to work similar to the shell script, by making the\n> files writable before calling rename().\n> \n> Another solution could be to do the \"chmod +x\" in mingw_rename().\n> This may be done in another commit, because\n> a) It improves git gc only when Git for Windows is used\n> on the client machine\n> b) Windows refuses to delete a file when the file is read-only.\n> So setting a file to read-only under Windows is a way for a user\n> to protect it from being deleted.\n> Changing the behaviour of rename() globally may not be what we want.\n> \n> Reported-by: Jochen Haag \n> Signed-off-by: Torsten Bögershausen \n> ---\n> builtin/repack.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index ba66c6e..033b4c2 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -324,6 +324,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n> \t\t\t\tchmod(fname_old, statbuffer.st_mode);\n> \t\t\t}\n> +\t\t\tif (!stat(fname, &statbuffer)) {\n> +\t\t\t\tstatbuffer.st_mode |= (S_IWUSR | S_IWGRP | S_IWOTH);\n> +\t\t\t\tchmod(fname, statbuffer.st_mode);\n> +\t\t\t}\n\nAre we sure that the file in question can never be a symlink? Because\nif it is, we'll end up changing the permissions on the wrong file. In\ngeneral, I'm wary of changing permissions on a file to suit Windows's\nrename because of the symlink issue and the security issues that can\nresult. Hard links probably have the same issue.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"233729","messageId":"alpine.DEB.1.00.1401242318060.14982@s15462909.onlinehome-server.info","threadId":"35728","inReplyTo":"20140124214023.GB58260@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH] repack.c: chmod +w before rename()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-01-24T22:24:36Z","receivedAt":"2014-01-24T22:24:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Brian,\n\nOn Fri, 24 Jan 2014, brian m. carlson wrote:\n\n> On Fri, Jan 24, 2014 at 10:05:14PM +0100, Torsten Bögershausen wrote:\n> > diff --git a/builtin/repack.c b/builtin/repack.c\n> > index ba66c6e..033b4c2 100644\n> > --- a/builtin/repack.c\n> > +++ b/builtin/repack.c\n> > @@ -324,6 +324,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> > \t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n> > \t\t\t\tchmod(fname_old, statbuffer.st_mode);\n> > \t\t\t}\n> > +\t\t\tif (!stat(fname, &statbuffer)) {\n> > +\t\t\t\tstatbuffer.st_mode |= (S_IWUSR | S_IWGRP | S_IWOTH);\n> > +\t\t\t\tchmod(fname, statbuffer.st_mode);\n> > +\t\t\t}\n> \n> Are we sure that the file in question can never be a symlink?\n\nOn Windows: yes. Otherwise, no.\n\nHaving said that, the files in question are files generated by Git, so you\nreally would have to tamper with things in order to get into trouble.\n\n> Because if it is, we'll end up changing the permissions on the wrong\n> file.\n\nIn any case, I'd rather change the permissions only when the rename\nfailed. *And* I feel uncomfortable ignoring the return value...\n\n> In general, I'm wary of changing permissions on a file to suit Windows's\n> rename because of the symlink issue and the security issues that can\n> result.\n\nI agree on the Windows issue.\n\n> Hard links probably have the same issue.\n\nNo, hard links have their own permissions. If you change the permissions\non a hardlink, any other hardlinks to the same content are unaffected.\n\nCiao,\nJohannes\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/groups/opt_out.\n"},{"id":"233732","messageId":"20140124224911.GC58260@vauxhall.crustytoothpaste.net","threadId":"35728","inReplyTo":"alpine.DEB.1.00.1401242318060.14982@s15462909.onlinehome-server.info","subject":"Re: [PATCH] repack.c: chmod +w before rename()","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-01-24T22:49:13Z","receivedAt":"2014-01-24T22:49:13Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Jan 24, 2014 at 11:24:36PM +0100, Johannes Schindelin wrote:\n> > In general, I'm wary of changing permissions on a file to suit Windows's\n> > rename because of the symlink issue and the security issues that can\n> > result.\n> \n> I agree on the Windows issue.\n\nI personally feel that if Windows needs help to change permissions for a\nrename, that code should only ever be used on Windows. Doesn't\nmingw_rename automatically do this anyway, and if it doesn't, shouldn't\nwe put the code there instead? Furthermore, it makes me very nervous to\nmake the file 666. Isn't 644 enough?\n\n> > Hard links probably have the same issue.\n> \n> No, hard links have their own permissions. If you change the permissions\n> on a hardlink, any other hardlinks to the same content are unaffected.\n\nNot according to link(2):\n\n This new name may be used exactly as the old one for any operation;\n both names refer to the same file (and so have the same permissions\n and ownership) and it is impossible to tell which name was the\n \"original\".\n\nMy testing confirms this.\n\nMore importantly, while one might not want to symlink a pack frequently,\ngit clone -l does use hard links. This means that if a local clone is\nmade somewhere, and then the original repository is repacked and hits\nthis case, the clone is now vulnerable to tampering by a malicious user\n(assuming that user can read the appropriate directory). So unless I'm\nreading this wrong, this patch would cause security problems on all Unix\nsystems.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"233734","messageId":"xmqq1tzxq7ml.fsf@gitster.dls.corp.google.com","threadId":"35728","inReplyTo":"201401242205.16313.tboegi@web.de","subject":"Re: [PATCH] repack.c: chmod +w before rename()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-24T23:31:46Z","receivedAt":"2014-01-24T23:31:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> Another solution could be to do the \"chmod +x\" in mingw_rename().\n> This may be done in another commit, because\n> a) It improves git gc only when Git for Windows is used\n> on the client machine\n> b) Windows refuses to delete a file when the file is read-only.\n> So setting a file to read-only under Windows is a way for a user\n> to protect it from being deleted.\n> Changing the behaviour of rename() globally may not be what we want.\n\nBut doesn't this affect non Windows platforms in negative ways? We\nunconditionally run stat(2) and chmod(2) unnecessarily, and we leave\nthe resulting file writable when it originally may have been marked\nread-only on purpose.\n\nAlso, it feels to me that doing this in mingw_rename() the right\napproach in the first place. If the user wants \"git mv a b\" to\nrename \"a\" in the working tree and if that path is read-only, what\nhappens, and what should happen? Does \"chmod -w\" on a file in the\nworking tree mean \"I want to make sure nobody accidentally writes to\nit, and also I want to protect it from being renamed or deleted\"?\n\nSo perhaps mingw_rename() can be taught to\n\n - First try to just do rename(), and if it succeeds, happily return\n without doing anything extra.\n\n - If it fails, then\n\n - check if the failure is due to read-only-ness (optional: if you\n cannot reliably tell this from the error code, do not bother),\n and if not, return failure.\n\n - otherwise, stat() to grab the current permission bits, do the\n chmod(), retry the rename, then restore the permission bits\n with another chmod(). Return failure if any of these steps\n fail.\n\nThat way, it can cover any call to rename(), be it for packfiles or\na path in the working tree, and you would need to pay the overhead\nof stat/chmod only when it matters, no?\n\n> Reported-by: Jochen Haag \n> Signed-off-by: Torsten Bögershausen \n> ---\n> builtin/repack.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index ba66c6e..033b4c2 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -324,6 +324,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\t\t\tstatbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);\n> \t\t\t\tchmod(fname_old, statbuffer.st_mode);\n> \t\t\t}\n> +\t\t\tif (!stat(fname, &statbuffer)) {\n> +\t\t\t\tstatbuffer.st_mode |= (S_IWUSR | S_IWGRP | S_IWOTH);\n> +\t\t\t\tchmod(fname, statbuffer.st_mode);\n> +\t\t\t}\n> \t\t\tif (rename(fname_old, fname))\n> \t\t\t\tdie_errno(_(\"renaming '%s' failed\"), fname_old);\n> \t\t\tfree(fname);\n> -- \n> 1.9.rc0.143.g6fd479e\n>\n> -- \n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/groups/opt_out.\n"},{"id":"233735","messageId":"xmqqwqhpot0p.fsf@gitster.dls.corp.google.com","threadId":"35728","inReplyTo":"alpine.DEB.1.00.1401242318060.14982@s15462909.onlinehome-server.info","subject":"Re: [PATCH] repack.c: chmod +w before rename()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-24T23:32:38Z","receivedAt":"2014-01-24T23:32:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> In any case, I'd rather change the permissions only when the rename\n> failed. *And* I feel uncomfortable ignoring the return value...\n\nGood judgement I'd agree with 100%.\n\nThanks.\n\n-- \n-- \n*** Please reply-to-all at all times ***\n*** (do not pretend to know who is subscribed and who is not) ***\n*** Please avoid top-posting. ***\nThe msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.\n\nYou received this message because you are subscribed to the Google\nGroups \"msysGit\" group.\nTo post to this group, send email to msysgit@googlegroups.com\nTo unsubscribe from this group, send email to\nmsysgit+unsubscribe@googlegroups.com\nFor more options, and view previous threads, visit this group at\nhttp://groups.google.com/group/msysgit?hl=en_US?hl=en\n\n--- \nYou received this message because you are subscribed to the Google Groups \"msysGit\" group.\nTo unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.\nFor more options, visit https://groups.google.com/groups/opt_out.\n"},{"id":"233737","messageId":"20140124235048.GA30427@glandium.org","threadId":"35728","inReplyTo":"20140124224911.GC58260@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH] repack.c: chmod +w before rename()","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2014-01-24T23:50:48Z","receivedAt":"2014-01-24T23:50:48Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Jan 24, 2014 at 10:49:13PM +0000, brian m. carlson wrote:\n> On Fri, Jan 24, 2014 at 11:24:36PM +0100, Johannes Schindelin wrote:\n> > > In general, I'm wary of changing permissions on a file to suit Windows's\n> > > rename because of the symlink issue and the security issues that can\n> > > result.\n> > \n> > I agree on the Windows issue.\n> \n> I personally feel that if Windows needs help to change permissions for a\n> rename, that code should only ever be used on Windows. Doesn't\n> mingw_rename automatically do this anyway, and if it doesn't, shouldn't\n> we put the code there instead? Furthermore, it makes me very nervous to\n> make the file 666. Isn't 644 enough?\n\nArguably, umask is supposed to take care of making things right. OTOH,\nsince it's the destination file that's the problem not the renamed file,\nthe equivalent to mv -f would be to unlink() that file first, not to change\nits permissions. That would work properly on unix too.\n\nMike\n"}]}