On Tue, Feb 19, 2013 at 7:59 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Adam Spiers <git@xxxxxxxxxxxxxx> writes:
>
>> Fix a corner case where check-ignore would segfault when run with the
>> '.' argument from the top level of a repository, due to prefix_path()
>> converting '.' into the empty string.
>
> The description does not match what I understand is happening from
> the original report, though.
Why not?
> The above is more like this, no?
>
> When check-ignore is run with the '.' argument from the top level of
> a repository, it fed an empty string to hash_name() in name-hash.c
> and caused a segfault, as the function kept reading forever past the
> end of the string.
The only difference I can see between the two is that yours has
changed the phrase order and gives a bit more information. I don't
see any disagreement between them though.
> A point to note is that it is not cleaer why it is a corner case to
> ask about a pathspec ".". It is a valid question "Is the whole tree
> ignored by default?", isn't it?
It's probably valid, but I'm not the best judge of that. It doesn't
make much sense to me why anyone would do that, and it seems very
unlikely it would be a common use case, therefore it's a corner case.
The next sentence then qualifies that:
>> It doesn't make much sense to
>> call check-ignore from the top level with '.' as a parameter, since
>> the top-level directory would never typically be ignored,
>
> And this sounds like a really bad excuse.
An excuse for what? The final part of that sentence which you trimmed
made it clear that it was not an excuse for the segfault. Your choice
of wording here sounds more like a personal attack than a technical
discussion - presumably unintentional, so I'll choose not to take
offense.
> If it were "it does not
> make *any* sense ... because the top level is *never* ignored", then
> the patch is a perfectly fine optimization that happens to work
> around the problem, but the use of "much" and "typically" is a sure
> sign that the design of the fix is iffy. It also shows that the
> patch is not a fix, but is sweeping the problem under the rug, if
> there were a valid use case to set the top level to be ignored.
There *may* be a valid use case to set the top level to be ignored.
*I* can't think of one personally, therefore it doesn't make sense to
me to do so, but my intention was to avoid imposing my own personal
judgement on everyone else by preventing people from doing that.
However, in that case I now realise that check-ignore should report a
match, and my proposed patches do not do that. Perhaps that is what
you meant by "sweeping the problem under the rug"?
> I wonder what would happen if we removed that "&& path[0]" check
> from the caller, not add the "assume the top is never ignored"
> workaround, and do something like this to the location that causes
> segv, so that it can give an answer when asked to hash an empty
> string?
>
> name-hash.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/name-hash.c b/name-hash.c
> index d8d25c2..942c459 100644
> --- a/name-hash.c
> +++ b/name-hash.c
> @@ -24,11 +24,11 @@ static unsigned int hash_name(const char *name, int namelen)
> {
> unsigned int hash = 0x123;
>
> - do {
> + while (namelen--) {
> unsigned char c = *name++;
> c = icase_hash(c);
> hash = hash*101 + c;
> - } while (--namelen);
> + }
> return hash;
> }
Yep, that makes sense - that's what I meant when I said I was
wondering "whether hash_name() should do stricter input validation".
> Does the callchain that goes down to this function have other places
> that assume they will never see an empty string, like this function
> does, which I _think_ is the real issue?
Good question. In the absence of a proper audit for similar issues,
it definitely makes sense to defensively program hash_name() (and any
other low-level functions which accept path strings).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html