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

From: erik elfström <erik.elfstrom@gmail.com>
Date: Apr 28, 2015, 19:28 UTC
Message-ID: <CAMpP7NY76Tj_=_6Evxvox20OmQ5LnNiK1j4VHLd0jKhRmci51g@mail.gmail.com>
In-Reply-To: <20150428060222.GK24580@peff.net>

On Tue, Apr 28, 2015 at 8:02 AM, Jeff King <peff@peff.net> wrote:

Show 8 quoted lines

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