threads / patch / 13370

patchBe more careful with objects directory permissions on clone

Subject: [PATCH] Be more careful with objects directory permissions on clone

## tl;dr

10 messages between May 4, 2008 and May 6, 2008. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Mark Hills· May 4, 2008, 11:37 UTC · lore

Honour the setgid and umask when re-creating the objects directory at the destination.

cpio in copy-pass mode aims to copy file permissions which causes this problem and cannot be disabled. Be explicit by copying the directory structure first, honouring the permissions at the destination, then copy the files with their existing read-only permissions.

Signed-off-by: Mark Hills <mark@pogo.org.uk>
---
  git-clone.sh |    5 ++++-
  1 files changed, 4 insertions(+), 1 deletions(-)
Show changes to git-clone.sh +4 −1
diff --git a/git-clone.sh b/git-clone.sh
index 8c7fc7f..53c7e06 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -334,7 +334,10 @@ yes)
  			fi
  		fi &&
  		cd "$repo" &&
-		find objects -depth -print | cpio $cpio_quiet_flag -pumd$l "$GIT_DIR/" || \
+		# Create dirs using umask and permissions and destination
+		find objects -type d -print | (cd "$GIT_DIR" && xargs mkdir -p) &&
+		# Copy 0444 permissions on files
+		find objects -type f -print | cpio $cpio_quiet_flag -pumd$l "$GIT_DIR/" || \
  			exit 1
  	fi
  	git-ls-remote "$repo" >"$GIT_DIR/CLONE_HEAD" || exit 1

-- 
1.5.5.1.126.g48d0c
Johannes Sixt· May 5, 2008, 09:01 UTC · re: Mark Hills · lore

Re: [PATCH] Be more careful with objects directory permissions on clone

Mark Hills schrieb:
Show 7 quoted lines
> Honour the setgid and umask when re-creating the objects directory
> at the destination.
> 
> cpio in copy-pass mode aims to copy file permissions which causes this
> problem and cannot be disabled. Be explicit by copying the directory
> structure first, honouring the permissions at the destination, then copy
> the files with their existing read-only permissions.
...
Show 8 quoted lines
>          cd "$repo" &&
> -        find objects -depth -print | cpio $cpio_quiet_flag -pumd$l
> "$GIT_DIR/" || \
> +        # Create dirs using umask and permissions and destination
> +        find objects -type d -print | (cd "$GIT_DIR" && xargs mkdir -p) &&
> +        # Copy 0444 permissions on files
> +        find objects -type f -print | cpio $cpio_quiet_flag -pumd$l
> "$GIT_DIR/" || \
Wouldn't that be better:
	find objects ! -type d -print | cpio ...
?
-- Hannes
Mark Hills· May 5, 2008, 09:56 UTC · re: Johannes Sixt · lore

Re: [PATCH] Be more careful with objects directory permissions on clone

On Mon, 5 May 2008, Johannes Sixt wrote:
Show 23 quoted lines
> Mark Hills schrieb:
>> Honour the setgid and umask when re-creating the objects directory
>> at the destination.
>>
>> cpio in copy-pass mode aims to copy file permissions which causes this
>> problem and cannot be disabled. Be explicit by copying the directory
>> structure first, honouring the permissions at the destination, then copy
>> the files with their existing read-only permissions.
> ...
>>          cd "$repo" &&
>> -        find objects -depth -print | cpio $cpio_quiet_flag -pumd$l
>> "$GIT_DIR/" || \
>> +        # Create dirs using umask and permissions and destination
>> +        find objects -type d -print | (cd "$GIT_DIR" && xargs mkdir -p) &&
>> +        # Copy 0444 permissions on files
>> +        find objects -type f -print | cpio $cpio_quiet_flag -pumd$l
>> "$GIT_DIR/" || \
>
> Wouldn't that be better:
>
> 	find objects ! -type d -print | cpio ...
>
> ?

This was my first suggestion, unfortunately it shows up broken behaviour in all but the latest version of cpio. It creates 0700 directory permissions which is even worse.

I didn't find the later versions of cpio to be widespread, so I wrote the patch to work in all cases.

See the thread 'git-clone file permissions and cpio'.
Mark
Johannes Sixt· May 5, 2008, 10:07 UTC · re: Mark Hills · lore

Re: [PATCH] Be more careful with objects directory permissions on clone

Mark Hills schrieb:
Show 22 quoted lines
> On Mon, 5 May 2008, Johannes Sixt wrote:
> 
>> Mark Hills schrieb:
>>>          cd "$repo" &&
>>> -        find objects -depth -print | cpio $cpio_quiet_flag -pumd$l
>>> "$GIT_DIR/" || \
>>> +        # Create dirs using umask and permissions and destination
>>> +        find objects -type d -print | (cd "$GIT_DIR" && xargs mkdir
>>> -p) &&
>>> +        # Copy 0444 permissions on files
>>> +        find objects -type f -print | cpio $cpio_quiet_flag -pumd$l
>>> "$GIT_DIR/" || \
>>
>> Wouldn't that be better:
>>
>>     find objects ! -type d -print | cpio ...
>>
>> ?
> 
> This was my first suggestion, unfortunately it shows up broken behaviour
> in all but the latest version of cpio. It creates 0700 directory
> permissions which is even worse.

Sorry, I should have mentioned that I meant to keep the 'find | xargs mkdir' and replace only the second 'find | cpio'.

-- Hannes
Mark Hills· May 5, 2008, 10:25 UTC · re: Johannes Sixt · lore

Re: [PATCH] Be more careful with objects directory permissions on clone

On Mon, 5 May 2008, Johannes Sixt wrote:
Show 26 quoted lines
> Mark Hills schrieb:
>> On Mon, 5 May 2008, Johannes Sixt wrote:
>>
>>> Mark Hills schrieb:
>>>>          cd "$repo" &&
>>>> -        find objects -depth -print | cpio $cpio_quiet_flag -pumd$l
>>>> "$GIT_DIR/" || \
>>>> +        # Create dirs using umask and permissions and destination
>>>> +        find objects -type d -print | (cd "$GIT_DIR" && xargs mkdir
>>>> -p) &&
>>>> +        # Copy 0444 permissions on files
>>>> +        find objects -type f -print | cpio $cpio_quiet_flag -pumd$l
>>>> "$GIT_DIR/" || \
>>>
>>> Wouldn't that be better:
>>>
>>>     find objects ! -type d -print | cpio ...
>>>
>>> ?
>>
>> This was my first suggestion, unfortunately it shows up broken behaviour
>> in all but the latest version of cpio. It creates 0700 directory
>> permissions which is even worse.
>
> Sorry, I should have mentioned that I meant to keep the 'find | xargs
> mkdir' and replace only the second 'find | cpio'.
Ah, I see.

I tend to think that an inclusive match has less scope for future bad behaviour than an exclusive match. Have I missed the possiblity that there may be content in the objects directory which is not a directory or file?

Mark
Johannes Sixt· May 5, 2008, 10:42 UTC · re: Mark Hills · lore

Re: [PATCH] Be more careful with objects directory permissions on clone

Mark Hills schrieb:
Show 35 quoted lines
> On Mon, 5 May 2008, Johannes Sixt wrote:
> 
>> Mark Hills schrieb:
>>> On Mon, 5 May 2008, Johannes Sixt wrote:
>>>
>>>> Mark Hills schrieb:
>>>>>          cd "$repo" &&
>>>>> -        find objects -depth -print | cpio $cpio_quiet_flag -pumd$l
>>>>> "$GIT_DIR/" || \
>>>>> +        # Create dirs using umask and permissions and destination
>>>>> +        find objects -type d -print | (cd "$GIT_DIR" && xargs mkdir
>>>>> -p) &&
>>>>> +        # Copy 0444 permissions on files
>>>>> +        find objects -type f -print | cpio $cpio_quiet_flag -pumd$l
>>>>> "$GIT_DIR/" || \
>>>>
>>>> Wouldn't that be better:
>>>>
>>>>     find objects ! -type d -print | cpio ...
>>>>
>>>> ?
>>>
>>> This was my first suggestion, unfortunately it shows up broken behaviour
>>> in all but the latest version of cpio. It creates 0700 directory
>>> permissions which is even worse.
>>
>> Sorry, I should have mentioned that I meant to keep the 'find | xargs
>> mkdir' and replace only the second 'find | cpio'.
> 
> Ah, I see.
> 
> I tend to think that an inclusive match has less scope for future bad
> behaviour than an exclusive match. Have I missed the possiblity that
> there may be content in the objects directory which is not a directory
> or file?
Theoretically, you could have symbolic links to loose objects or packs.
Generally, you shouldn't change the behavior of a program more than necessary.
-- Hannes
Mark Hills· May 5, 2008, 16:46 UTC · re: Johannes Sixt · lore

Re: [PATCH] Be more careful with objects directory permissions on clone

On Mon, 5 May 2008, Johannes Sixt wrote:
> Mark Hills schrieb:
Show 9 quoted lines
>> I tend to think that an inclusive match has less scope for future bad 
>> behaviour than an exclusive match. Have I missed the possiblity that 
>> there may be content in the objects directory which is not a directory 
>> or file?
>
> Theoretically, you could have symbolic links to loose objects or packs.
>
> Generally, you shouldn't change the behavior of a program more than 
> necessary.

Okay, here's a revised patch (I made a note about the cpio bug in the commit message too).

Thanks,
Mark
Be more careful with objects directory permissions on clone

Honour the setgid and umask when re-creating the objects directory at the destination.

cpio in copy-pass mode aims to copy file permissions which causes this problem and cannot be disabled. Be explicit by copying the directory structure first, honouring the permissions at the destination, then copy the files with 0444 permissions. This also avoids bugs in some versions of cpio.

Signed-off-by: Mark Hills <mark@pogo.org.uk>
---
  git-clone.sh |    5 ++++-
  1 files changed, 4 insertions(+), 1 deletions(-)
Show changes to git-clone.sh +4 −1
diff --git a/git-clone.sh b/git-clone.sh
index 8c7fc7f..9d88d1c 100755
--- a/git-clone.sh
+++ b/git-clone.sh
@@ -334,7 +334,10 @@ yes)
  			fi
  		fi &&
  		cd "$repo" &&
-		find objects -depth -print | cpio $cpio_quiet_flag -pumd$l "$GIT_DIR/" || \
+		# Create dirs using umask and permissions and destination
+		find objects -type d -print | (cd "$GIT_DIR" && xargs mkdir -p) &&
+		# Copy existing 0444 permissions on content
+		find objects ! -type d -print | cpio $cpio_quiet_flag -pumd$l "$GIT_DIR/" || \
  			exit 1
  	fi
  	git-ls-remote "$repo" >"$GIT_DIR/CLONE_HEAD" || exit 1

-- 
1.5.5.1.126.geeac
Junio C Hamano· May 6, 2008, 06:53 UTC · re: Mark Hills · lore

Re: [PATCH] Be more careful with objects directory permissions on clone

Mark Hills <mark@pogo.org.uk> writes:
Show 14 quoted lines
> diff --git a/git-clone.sh b/git-clone.sh
> index 8c7fc7f..9d88d1c 100755
> --- a/git-clone.sh
> +++ b/git-clone.sh
> @@ -334,7 +334,10 @@ yes)
>  			fi
>  		fi &&
>  		cd "$repo" &&
> -		find objects -depth -print | cpio $cpio_quiet_flag -pumd$l "$GIT_DIR/" || \
> +		# Create dirs using umask and permissions and destination
> +		find objects -type d -print | (cd "$GIT_DIR" && xargs mkdir -p) &&
> +		# Copy existing 0444 permissions on content
> +		find objects ! -type d -print | cpio $cpio_quiet_flag -pumd$l "$GIT_DIR/" || \
>  			exit 1
Looks much better.  Thanks.
Jakub Narebski· May 6, 2008, 07:25 UTC · re: Mark Hills · lore

Re: [PATCH] Be more careful with objects directory permissions on clone

Mark Hills wrote:
Show 5 quoted lines
> -               find objects -depth -print | cpio $cpio_quiet_flag -pumd$l "$GIT_DIR/" || \
> +               # Create dirs using umask and permissions and destination
> +               find objects -type d -print | (cd "$GIT_DIR" && xargs mkdir -p) &&
> +               # Copy existing 0444 permissions on content
> +               find objects ! -type d -print | cpio $cpio_quiet_flag -pumd$l "$GIT_DIR/" || \

By the way, it is important that previous version had -depth, and proposed one doesn't? Was it about creating directories before files?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
Johannes Sixt· May 6, 2008, 08:08 UTC · re: Jakub Narebski · lore

Re: [PATCH] Be more careful with objects directory permissions on clone

Jakub Narebski schrieb:
Show 10 quoted lines
> Mark Hills wrote:
> 
>> -               find objects -depth -print | cpio $cpio_quiet_flag -pumd$l "$GIT_DIR/" || \
>> +               # Create dirs using umask and permissions and destination
>> +               find objects -type d -print | (cd "$GIT_DIR" && xargs mkdir -p) &&
>> +               # Copy existing 0444 permissions on content
>> +               find objects ! -type d -print | cpio $cpio_quiet_flag -pumd$l "$GIT_DIR/" || \
> 
> By the way, it is important that previous version had -depth, and
> proposed one doesn't? Was it about creating directories before files?

-depth means that directory names are listed *after* their content. Consequently, it was about setting modification times and permissions on directories *after* all of their content is created at the destination.

The intent of the new version is to not copy permissions of directories; and since the modification times don't matter, the absence of -depth is ok.

-- Hannes

← back to recent threads