Re: [PATCH v4 1/2] fetch-pack: redact packfile urls in traces

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 28, 2021, 22:46 UTC
Message-ID: <xmqqa6ispy28.fsf@gitster.g>
In-Reply-To: <CANQMx9WFKJAGF+7zti8+-b2je9sFuNxwOx-LCPtEoGCea54Mdw@mail.gmail.com>

Ivan Frade <ifrade@google.com> writes:

Show 5 quoted lines

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

Ah, I see. This is merely a tracing, so we might benefit from a generalized version of redactor, and from that point of view, the use of strstr and stopping at the whitespace do make sort-of sense to me, but then we lack any attempt to redact more than one instance of URL in a packet, so the generalization may have quite a limited usefulness.

> Next patch version follows these suggestions to look for a packfile-uri line.

Yeah, I think that is a good way to go, at least for now. When we want a more general one, we can revisit it, but not now.

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

I was mostly wondering about the cost of determining "if it looks like?". But we do this only for the protocol header part, so we won't have thousands of attempts to match, I guess. Oh, or if we also do this for the ref advertisement packets, then we might have quite a many. Hmph.

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

Yeah, that is quite a wise design decision, I would think.

Thanks.