Re: [PATCH v2 1/1] pack-bitmap.c: avoid exposing absolute paths

[Taylor Blau <me@ttaylorr.com>]

On Wed, Nov 02, 2022 at 08:56:05PM +0800, Teng Long wrote:
> From: Teng Long <dyroneteng@gmail.com>
>
> In "open_midx_bitmap_1()" and "open_pack_bitmap_1()", when we find that
> there are multiple bitmaps, we will only open the first one and then
> leave warnings about the remaining pack information, the information
> will contain the absolute path of the repository, for example in a
> alternates usage scenario. So let's hide this kind of potentially
> sensitive information in this commit.
>
> Found-by: XingXin <moweng.xx@antgroup.com>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  pack-bitmap.c           | 12 ++++++++----
>  t/t5310-pack-bitmaps.sh | 11 ++++++++---
>  2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 36134222d7a..a8c76056637 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -331,8 +331,9 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>  	if (bitmap_git->pack || bitmap_git->midx) {
>  		struct strbuf buf = STRBUF_INIT;
>  		get_midx_filename(&buf, midx->object_dir);
> -		/* ignore extra bitmap file; we can only handle one */
> -		warning("ignoring extra bitmap file: %s", buf.buf);

OK... here we're getting rid of the user-visible warning, which makes
sense and is the point of this patch.

> +		/* ignore extra midx bitmap files; we can only handle one */
> +		trace2_data_string("bitmap", the_repository,
> +				   "ignoring extra midx bitmap file", basename(buf.buf));

But we replace it with a trace2_data_string() that only includes the
basename? I'd think that the point of moving this warning into
trace2-land is that we could emit the full path without worrying about
who sees it, since trace2 data is typically not plumbed through to
end-users.

IOW, I would have expected to see a patch something along the lines of:

> -		/* ignore extra bitmap file; we can only handle one */
> -		warning("ignoring extra bitmap file: %s", buf.buf);
> +		/* ignore extra midx bitmap files; we can only handle one */
> +		trace2_data_string("bitmap", the_repository,
> +				   "ignoring extra midx bitmap file", buf.buf);

> @@ -402,8 +403,9 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>  	}
>
>  	if (bitmap_git->pack || bitmap_git->midx) {
> -		/* ignore extra bitmap file; we can only handle one */
> -		warning("ignoring extra bitmap file: %s", packfile->pack_name);
> +		/* ignore extra bitmap files; we can only handle one */
> +		trace2_data_string("bitmap", the_repository,
> +				   "ignoring extra bitmap file", basename(packfile->pack_name));

Same note here.

> +	trace2_data_string("bitmap", the_repository, "opened bitmap file",
> +			   basename(packfile->pack_name));

If we get later bitmap-related information in the trace2 stream, we know
that we opened a bitmap. And at the moment we read a bitmap, there is
only one such bitmap in the repository.

I suppose that this is race-proof in the sense that if the bitmaps
change after reading, we can still usefully debug the trace2 stream
after the fact.

So even though my first reaction was that this was unnecessary, on
balance I do find it useful.

> -test_expect_success 'complains about multiple pack bitmaps' '
> +test_expect_success 'complains about multiple pack bitmaps when debugging by trace2' '
>  	rm -fr repo &&
>  	git init repo &&
>  	test_when_finished "rm -fr repo" &&
> @@ -420,8 +420,13 @@ test_expect_success 'complains about multiple pack bitmaps' '
>  		test_line_count = 2 packs &&
>  		test_line_count = 2 bitmaps &&
>
> -		git rev-list --use-bitmap-index HEAD 2>err &&
> -		grep "ignoring extra bitmap file" err
> +		ls -tr .git/objects/pack | grep -e "^pack-.*\.pack$" > sorted-packs &&
> +		ignored_pack="$(cat sorted-packs | awk 'NR==1{print}')" &&
> +		open_pack="$(cat sorted-packs | awk 'NR==2{print}')" &&

Hmmph. This test only passes if 'ls -tr' gives us the packs in order
that they are read by readdir(), which doesn't seem all that portable to
me. At the very least, it is tightly coupled to the implementation of
open_pack_bitmap() and friends.

> +		GIT_TRACE2_PERF=1 git rev-list --use-bitmap-index HEAD 2>err &&
> +		grep "opened bitmap file:$opened_pack" err &&
> +		grep "ignoring extra bitmap file:$ignored_pack" err

Do we necessarily care about which .bitmap is read and which isn't? The
existing test doesn't look too closely, which I think is fine (though as
the author of that test, I might be biased ;-)).

I would be equally happy to write:

> +		GIT_TRACE2_PERF=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&
> +		grep "opened bitmap" trace2.txt &&
> +		grep "ignoring extra bitmap" trace2.txt

Thanks,
Taylor