git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Taylor Blau <me@ttaylorr.com>
To: Teng Long <dyroneteng@gmail.com>
Cc: avarab@gmail.com, derrickstolee@github.com, git@vger.kernel.org,
	gitster@pobox.com, me@ttaylorr.com, tenglong.tl@alibaba-inc.com,
	peff@peff.net, XingXin <moweng.xx@antgroup.com>
Subject: Re: [PATCH v2 1/1] pack-bitmap.c: avoid exposing absolute paths
Date: Wed, 2 Nov 2022 21:16:40 -0400	[thread overview]
Message-ID: <Y2MWeE2f/P1iXPCY@nand.local> (raw)
In-Reply-To: <87a494e5ac0cc992689944ab13600d097c51e54a.1667393419.git.dyroneteng@gmail.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

  reply	other threads:[~2022-11-03  1:16 UTC|newest]

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-26  7:09 [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths Teng Long
2022-08-26  7:09 ` [PATCH 1/1] " Teng Long
2022-08-26 16:34 ` [PATCH 0/1] " Junio C Hamano
2022-08-29  2:48   ` Teng Long
2022-10-26 21:42     ` Taylor Blau
2022-10-26 23:19       ` Ævar Arnfjörð Bjarmason
2022-10-31 13:20         ` Teng Long
2022-10-27 20:45       ` Jeff King
2022-10-30 18:42         ` Taylor Blau
2022-10-31 12:22           ` [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths Taylor Blau <me@ttaylorr.com> writes: Teng Long
2022-11-02  5:37         ` [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths Teng Long
2022-11-02  7:54           ` Jeff King
2022-11-02 13:52             ` Teng Long
2022-10-31 13:13       ` Teng Long
2022-11-03  1:00         ` Taylor Blau
2022-11-02  9:20 ` Ævar Arnfjörð Bjarmason
2022-11-02 13:04   ` Teng Long
2022-11-02 12:56 ` [PATCH v2 " Teng Long
2022-11-02 12:56   ` [PATCH v2 1/1] " Teng Long
2022-11-03  1:16     ` Taylor Blau [this message]
2022-11-03  9:35       ` Teng Long
2022-11-05  0:35         ` Taylor Blau
2022-11-03  1:21   ` [PATCH v2 0/1] " Taylor Blau
2022-11-03  8:42     ` Teng Long
2022-11-04  3:17   ` [PATCH v3 0/2] " Teng Long
2022-11-04  3:17     ` [PATCH v3 1/2] " Teng Long
2022-11-04 22:11       ` Taylor Blau
2022-11-04  3:17     ` [PATCH v3 2/2] pack-bitmap.c: remove unnecessary "open_pack_index()" calls Teng Long
2022-11-04 22:09       ` Taylor Blau
2022-11-04 22:13     ` [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths Taylor Blau
2022-11-10  7:10     ` Teng Long
2022-11-10  7:10       ` [PATCH v3 1/2] pack-bitmap.c: remove unnecessary "open_pack_index()" calls Teng Long
2022-11-14 22:03         ` Jeff King
2022-11-14 22:14           ` Taylor Blau
2022-11-14 22:31             ` Jeff King
2022-11-14 22:50               ` Taylor Blau
2022-11-10  7:10       ` [PATCH v3 2/2] pack-bitmap.c: avoid exposing absolute paths Teng Long
2022-11-11 22:26       ` [PATCH v3 0/2] " Taylor Blau
2022-11-14 22:23         ` Jeff King
2022-11-17 14:19           ` Teng Long
2022-11-17 15:03             ` Jeff King
2022-11-17 21:57               ` Taylor Blau
2022-11-21  3:27                 ` Teng Long
2022-11-21 12:16     ` [PATCH v4 0/4] " Teng Long
2022-11-21 12:16       ` [PATCH v4 1/4] pack-bitmap.c: remove unnecessary "open_pack_index()" calls Teng Long
2022-11-21 12:16       ` [PATCH v4 2/4] pack-bitmap.c: avoid exposing absolute paths Teng Long
2022-11-21 12:16       ` [PATCH v4 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing Teng Long
2022-11-21 23:27         ` Junio C Hamano
2022-11-28 13:09           ` Teng Long
2022-11-21 12:16       ` [PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found Teng Long
2022-11-21 19:09         ` Jeff King
2022-11-21 23:29           ` Junio C Hamano
2022-11-28 12:29             ` Teng Long
2022-11-28 12:37           ` Teng Long
2022-11-29  1:27             ` Jeff King
2022-11-29 13:14               ` Teng Long
2022-11-21 19:04       ` [PATCH v4 0/4] pack-bitmap.c: avoid exposing absolute paths Jeff King
2022-11-28 12:48         ` Teng Long
2022-11-28 14:09       ` [PATCH v5 " Teng Long
2022-11-28 14:09         ` [PATCH v5 1/4] pack-bitmap.c: remove unnecessary "open_pack_index()" calls Teng Long
2022-11-28 14:09         ` [PATCH v5 2/4] pack-bitmap.c: avoid exposing absolute paths Teng Long
2022-11-28 14:09         ` [PATCH v5 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing Teng Long
2022-11-28 23:26           ` Taylor Blau
2022-11-29 13:17             ` Teng Long
2022-11-28 14:09         ` [PATCH v5 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found Teng Long
2022-11-28 23:30         ` [PATCH v5 0/4] pack-bitmap.c: avoid exposing absolute paths Taylor Blau
2022-11-29 13:21           ` Teng Long

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y2MWeE2f/P1iXPCY@nand.local \
    --to=me@ttaylorr.com \
    --cc=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=dyroneteng@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=moweng.xx@antgroup.com \
    --cc=peff@peff.net \
    --cc=tenglong.tl@alibaba-inc.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).