On Wed, Dec 06, 2023 at 02:40:35PM -0500, Jeff King wrote:
> 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.
> 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
> 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.
Attachment:
signature.asc
Description: PGP signature
