Re: [PATCH v4 1/2] fetch-pack: redact packfile urls in traces
- From
Ivan Frade <ifrade@google.com>
- Date
- Oct 28, 2021, 22:15 UTC
- Message-ID
- <CANQMx9WFKJAGF+7zti8+-b2je9sFuNxwOx-LCPtEoGCea54Mdw@mail.gmail.com>
- In-Reply-To
- <xmqq35omt11f.fsf@gitster.g>
On Wed, Oct 27, 2021 at 6:01 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> > "Ivan Frade via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Ivan Frade <ifrade@google.com> > >
> Just a curiosity. Do we call these packfile URI, or packfile URL?
The feature is "packfile URI" (and the section is called so in the protocol). I changed all "url" to "uri".
Show 17 quoted lines
> > diff --git a/pkt-line.c b/pkt-line.c
> > index 2dc8ac274bd..ba0a2d65f0c 100644
> > --- a/pkt-line.c
> > +++ b/pkt-line.c
> > @@ -370,6 +370,31 @@ int packet_length(const char lenbuf_hex[4])
> > return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
> > }
> >
> > +static char *find_url_path(const char* buffer, int *path_len)
> > +{
> > + const char *URL_MARK = "://";
> > + char *path = strstr(buffer, URL_MARK);
> > + if (!path)
> > + return NULL;
>
> Hmph, the format we expect is "<hash> <uri>"; don't we need to
> validate the leading <hash> followed by SP?I was trying to find a uri in a packet in general, not counting on the packfile-uri line format. That is probably an overgeneralization.
Next patch version follows these suggestions to look for a packfile-uri line.
Show 7 quoted lines
> > + if (path_len) {
> > + char *url_end = strchrnul(path, ' ');
>
> Is this because SP is not a valid character in packfile URI, or at
> this point in the callchain it would be encoded or something? The
> format we expect is "<hash> <uri>", so we shouldn't even have to
> look for SP but just redact everything to the end, no?Yes, now that we count on the packfile-uri line format, we can redact everything to the end and there is no need to return the length.
Show 16 quoted lines
> > - packet_trace(buffer, len, 0);
> > + if (options & PACKET_READ_REDACT_URL_PATH &&
> > + (url_path_start = find_url_path(buffer, &url_path_len))) {
> > + const char *redacted = "<redacted>";
> > + struct strbuf tracebuf = STRBUF_INIT;
> > + strbuf_insert(&tracebuf, 0, buffer, len);
> > + strbuf_splice(&tracebuf, url_path_start - buffer,
> > + url_path_len, redacted, strlen(redacted));
> > + packet_trace(tracebuf.buf, tracebuf.len, 0);
> > + strbuf_release(&tracebuf);
>
> I briefly wondered if the repeated allocation (and more
> fundamentally, preparing the redacted copy of packet whether we are
> actually tracing the packet in the first place) is blindly wasting
> the resources too much, but this only happens in the protocol header
> part, so it might be OK.We only allocate and redact if it looks like a packfile-uri line, so it shouldn't happen too frequently.
Show 5 quoted lines
> Even if that is not the case, we should be able to update > fetch_pack.c::do_fetch_pack_v2() so that the REDACT_URL_PATH bit is > turned on in a much narrower region of code, right? Enable when we > enter the GET_PACK state and drop the bit when we are done with the > packfile URI packets, or something?
I move the set/unset of the redacting flag to the FETCH_GET_PACK around the "packfile-uris" section. There is no need to check every incoming packet for a packfile-uri line, we know when they should come.
Thanks,
Ivan