Re: [PATCH] object-name: reject too-deep recursive ancestor queries

From: Patrick Steinhardt <ps@pks.im>
Date: Dec 7, 2023, 06:52 UTC
Message-ID: <ZXFroM6x5xY8fszx@tanuki>
In-Reply-To: <20231206194035.GB103708@coredump.intra.peff.net>

On Wed, Dec 06, 2023 at 02:40:35PM -0500, Jeff King wrote:

Show 22 quoted lines

> On Fri, Nov 24, 2023 at 11:11:53AM +0100, Patrick Steinhardt wrote:
> 
> > > When we get "HEAD~~~~~~~~~^2~~~~~~" from the user, do we somehow try
> > > to create a file or a directory with that name and fail due to
> > > ENAMETOOLONG?
> > 
> > Sorry, this was a typo on my part. I didn't mean "revision", I meant
> > "reference" here. References are limited to at most 4kB on most
> > platforms due to filesystem limitations, whereas revisions currently
> > have no limits in place.
> 
> Even without filesystem limitations, references are effectively limited
> to 64kb due to the pkt-line format.
> 
> Revisions can be much longer than a reference, though. We accept
> "some_ref:some/path/in/tree", for instance[1].  I think you could argue
> that paths are likewise limited by the filesystem, though. Even on
> systems like Linux where paths can grow arbitrarily long (by descending
> and adding to the current directory), you're still limited in specifying
> a full pathname. And Git will always use the full path from the project
> root when creating worktree entries. Plus my recent tree-depth patches
> effectively limit us to 16MB in the default config.

I was only able to trigger these issues with _really_ long revisions, like hundreds of megabytes. But that's on a glibc system, other systems based on e.g. musl libc have a much smaller stack by default where these limits would be hit sooner.

Show 7 quoted lines

> So I think it might be reasonable to limit revision lengths just as a
> belt-and-suspenders against overflow attacks, etc. But I suspect that
> the limits we'd choose there might not match what we'd want for
> protection against stack exhaustion via recursion. E.g., I think 8k is
> probably the minimum I'd want for a revision ("my/4k/ref:my/4k/path").
> If one "~" character can create an expensive recursion, that might be
> too much.

Fair enough. I think combining the two approaches would be sensible as a defense-in-depth approach.

Patrick

Show 9 quoted lines

> So we probably need something like Taylor's patch anyway (or to switch
> to an iterative algorithm, though that might be tricky because of the
> way we parse). I agree it needs to handle "^", though.
> 
> -Peff
> 
> [1] There are other more exotic revisions, too. The most arbitrary-sized
>     that comes to mind is ":/some-string-to-match". I doubt anybody
>     would be too mad if that were limited to 8k or even 4k, though.