From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 12 May 2022 18:15:05 GMT
Subject: Re: [PATCH v4 1/7] archive: optionally add "virtual" files
Message-ID: <xmqqfslefwie.fsf@gitster.g>
In-Reply-To: <47ed5a2f-f4aa-1ec1-27c9-9b0b70eb8bca@web.de>

René Scharfe <l.s.r@web.de> writes:

> Good point.  write_tar_entry() actually normalizes the permission bits
> and applies tar.umask (0002 by default):
>
> 	if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
> 		*header.typeflag = TYPEFLAG_DIR;
> 		mode = (mode | 0777) & ~tar_umask;
> 	} else if (S_ISLNK(mode)) {
> 		*header.typeflag = TYPEFLAG_LNK;
> 		mode |= 0777;
> 	} else if (S_ISREG(mode)) {
> 		*header.typeflag = TYPEFLAG_REG;
> 		mode = (mode | ((mode & 0100) ? 0777 : 0666)) & ~tar_umask;

Yeah, this side seems to care only about u+x bit, so
"add-executable" as a separate option would fly we..

> But write_zip_entry() only normalizes (drops) the permission bits of
> non-executable files:
>
>                 attr2 = S_ISLNK(mode) ? ((mode | 0777) << 16) :
>                         (mode & 0111) ? ((mode) << 16) : 0;
>                 if (S_ISLNK(mode) || (mode & 0111))
>                         creator_version = 0x0317;
>
> attr2 corresponds to the field "external file attributes" mentioned in
> the ZIP format specification, APPNOTE.TXT.  It's interpreted based on
> the "version made by" (creator_version here); that 0x03 part above
> means "UNIX".  The default is MS-DOS (FAT filesystem), with effectivly
> no support for file permissions.
>
> So we currently leak permission bits of executable files into ZIP
> archives, but not tar files. :-|  Normalizing those to 0755 would be
> more consistent.

Yup.

>> For tracked paths, we probably are normalizing the blobs
>> between 0644 and 0755 way before the values are passed as "mode"
>> parameter to the write_entry() functions, but for these extra files,
>> there is no such massaging.
>
> Right, mode values from read_tree() pass through canon_mode(), so only
> untracked files (those appended with --add-file) are affected by the
> leakage mentioned above.

Thanks for sanity-checking.