Re: odb_mkstemp's 0444 permission broke write/delete access on AFP

["Torsten Bögershausen" <tboegi@web.de>]

On 17/02/15 17:58, Fairuzan Roslan wrote:
> 
>> On Feb 17, 2015, at 4:51 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>>
>> Fairuzan Roslan <fairuzan.roslan@gmail.com> writes:
>>
>>> $ git clone https://github.com/robbyrussell/oh-my-zsh.git
>>> Cloning into 'oh-my-zsh'...
>>> remote: Counting objects: 11830, done.
>>> remote: Total 11830 (delta 0), reused 0 (delta 0)
>>> Receiving objects: 100% (11830/11830), 2.12 MiB | 481.00 KiB/s, done.
>>> Resolving deltas: 100% (6510/6510), done.
>>> warning: unable to unlink /Volumes/installer/oh-my-zsh/.git/objects/pack/tmp_pack_zjPxuc: Operation not permitted
>>
>> This should be fixable from Git itself, by replacing the calls to
>> "unlink" with something like
>>
>> int unlink_or_chmod(...) {
>> 	if (unlink(...)) {
>> 		chmod(...); // give user write permission
>> 		return unlink(...);
>> 	}
>> }
>>
>> This does not add extra cost in the normal case, and would fix this
>> particular issue for afp shares. So, I think that would fix the biggest
>> problem for afp-share users without disturbing others. It seems
>> reasonable to me to do that unconditionnally.
>>
>>> $ rm -rf oh-my-zsh/.git/objects/pack/tmp_*
>>> rm: oh-my-zsh/.git/objects/pack/tmp_idx_oUN1sb: Operation not permitted
>>> rm: oh-my-zsh/.git/objects/pack/tmp_pack_zjPxuc: Operation not permitted
>>
>> What happens if you do "rm -fr oh-my-zsh/.git/objects/pack/" (i.e.
>> remove the directory, not the files)?
>>
>> If you can still remove the directory, then I'd say the solution above
>> could be sufficient: the user isn't supposed to interfer with the
>> content of .git/objects other than by using Git, and if he or she does,
>> then asking a chmod prior to an rm seems reasonable.
>>
>> If you can't, then it's another problematic use-case (basically, you
>> can't just "rm -fr" a whole clone), and then it deserves at least an
>> opt-in configuration to get writable pack files.
>>
>> (Unfortunately, I suspect we're in the later case)
>>
>>> If you insist on setting the tmp idx & pack file permission to 0444 at
>>> least give it a u+w permission whenever you try to unlink and rename
>>> it so it won’t fail.
>>
>> Yes. In case you hadn't guessed, this is precisely what I had in mind
>> when I asked "Is it a problem when using Git [...] or when trying to
>> remove files outside Git?".
>>
>> --
>> Matthieu Moy
>> http://www-verimag.imag.fr/~moy/
> 
> Yes. It’s a problem when using Git where it fails to unlink and rename the tmp idx and pack files.
> The reason I tries to rm -rf the tmp_idx_XXXXXX and tmp_pack_XXXXXX is to proof a point why Git fails
> 
> Perhaps my explanation wasn’t clear enough. Maybe it’s hard for you to understand without having to test it yourself on a AFP filesystem.
> 
> Let me explain why AFP filesystem is more strict and different from your typical filesystem like ext4,hfs+,etc.
> 
> $ mkdir testdir; chmod 0755 testdir; touch testdir/testfile; chmod 0444 testdir/testfile; ls -la testdir
> total 0
> drwxr-xr-x  1 user  staff  264 Feb 18 00:26 .
> drwx------  1 user  staff  264 Feb 18 00:26 ..
> -r--r--r--  1 user  staff    0 Feb 18 00:26 testfile
> 
> $ rm -rf testdir
> rm: testdir/testfile: Operation not permitted
> rm: testdir: Directory not empty
> 
This works on my system (Mac OS 10.9 as server and client)

> $ chmod +w testdir/testfile; ls -la testdir
> total 0
> drwxr-xr-x  1 riaf  staff  264 Feb 18 00:26 .
> drwx------  1 riaf  staff  264 Feb 18 00:26 ..
> -rw-r—r--  1 riaf  staff    0 Feb 18 00:26 testfile
> 
> $ rm -rf testdir <—— No error message
> 
> This show that you cannot delete a directory or a file without a write permission in AFP filesystem.
> 
> The problem with Git failing is not because its inability to delete a directory but its inability to unlink and rename tmp_idx_XXXXXX and tmp_pack_XXXXXX because those files were set to 0444 by odb_mkstemp.
> Try google for “Git AFP” and you will see a lot people are facing with the same problem.
Yes, (at least to my knowledge) you seem to be one of the first to report it here, thanks for that.
> 
> Regarding your suggestion, yes I think it would work but you have to take care (chmod) every calls that rename or unlink or delete files with 0444 permission.
> 
> Regards,
> Fairuzan
> 
The "right" solution is to make a wrapper function, and to re-define unlink() and rename() with help
of the preprocessor.

git-compat-util.h has an example for fopen(), so that can be used for a patch.

And no, as I can not reproduce it here, I can only help with reviews or so.