Re: [PATCH v4 1/7] archive: optionally add "virtual" files

From: René Scharfe <l.s.r@web.de>
Date: May 12, 2022, 16:16 UTC
Message-ID: <47ed5a2f-f4aa-1ec1-27c9-9b0b70eb8bca@web.de>
In-Reply-To: <xmqqzgjnkgy0.fsf@gitster.g>

Am 11.05.22 um 21:27 schrieb Junio C Hamano:

Show 17 quoted lines

> René Scharfe <l.s.r@web.de> writes:
>
>> Regarding file modes: We only effectively support the executable bit,
>> so an additional option --add-virtual-executable-file=<path>:<contents>
>> would suffice.
>
> While I do not think we want to support more than one "is it
> executable or not?" bit, I am not so sure about what the current
> code does, though, for these "not from a tree, but added as extra
> files" entries.
>
> If you add an extra file from an on-disk untracked file, the
> add_file_cb() callback picks up the full st.st_mode for the file,
> and write_archive_entries() in its loop over args->extra_files pass
> the full info->stat.st_mode down to write_entry(), which is used by
> archive-tar.c::write_tar_entry() to obtain mode bits pretty much
> as-is.

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;

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.

> 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.

René