Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root
From: Adam Spiers <hidden>
Date: 2016-06-15 22:56:12
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root · Adam Spiers <hidden>
- 2016-06-15 · Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root · Adam Spiers <hidden>
- 2016-06-15 · Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root · Adam Spiers <hidden>
On Tue, Feb 19, 2013 at 7:59 PM, Junio C Hamano [off-list ref] wrote:
Adam Spiers [off-list ref] writes:quoted
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:
quoted
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"?
quoted hunk ↗ jump to hunk
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).