Re: [PATCH v5 2/5] setup: sanity check file size in read_gitfile_gently

["erik elfström" <erik.elfstrom@gmail.com>]

On Tue, Apr 28, 2015 at 8:02 AM, Jeff King <peff@peff.net> wrote:
>
> My understanding is that PATH_MAX is set absurdly low on Windows
> systems (and doesn't actually represent the real limit of a path!).
> Since the value is picked arbitrarily anyway, could use something more
> independent (like 100K or something, which is large enough to be beyond
> absurd and small enough that a malloc isn't a big deal)?
>
> -Peff

I'm happy to set the limit to anything that makes everybody feel safe. I'll set
it to 1MB to be on the safe side.

I'm not sure though how the code (in general) is supposed to keep working if
a path can exceed PATH_MAX? A cursory search for PATH_MAX comes
up with char array sizes and check-and-die kind of things. If a path is longer
then surely we will be unable to handle it and abort in all sorts of places?

Are you only worried we might have a submodule with a too long path (that will
create various other problems in different codepaths) that we may mistakenly
clean (if it doesn't trigger any other abort earlier in the clean call
chain) or do
you want clean to keep working and do the right thing even in this case?

While digging around looking at this I also noticed that there is
another problem
I have overlooked previously.

read_gitfile_gently will call is_git_directory at the very end and it
contains the
following check at the very beginning:

    if (PATH_MAX <= len + strlen("/objects"))
        die("Too long path: %.*s", 60, suspect);

Now, this is good in the way that we will avoid mistakenly cleaning
stuff because
the path is too long but also bad because it makes read_gitfile_gently
behave very
ungently in this case.

I suspect I should make a gentle version of this also. The question is
what to do
in clean if the path is reported as too long? Abort? Avoid cleaning it
to be safe?
Ignore and clean it?

is_git_directory is also called from the new is_git_repository
directly but here I think
dying is ok since this path is a path in the working tree and if we
can't handle the paths
in the tree then there seem to be little point in trying to go on (as
opposed to when
some string in a file is too large for a path)

Thoughts?

/Erik