* [PATCH 0/2] silence clang-3.6 warnings @ 2015-01-28 17:53 Jeff King 2015-01-28 17:57 ` [PATCH 1/2] read_and_strip_branch: fix typo'd address-of operator Jeff King 2015-01-28 17:58 ` [PATCH 2/2] do not check truth value of flex arrays Jeff King 0 siblings, 2 replies; 6+ messages in thread From: Jeff King @ 2015-01-28 17:53 UTC (permalink / raw) To: git; +Cc: Junio C Hamano I was fooling around with clang-3.6 today (as opposed to 3.5, which is the stock "clang" shipped with Debian), and noticed two new warnings. The first one is a real bug, and the second is just a cleanup (though I agree with clang on it). [1/2]: read_and_strip_branch: fix typo'd address-of operator [2/2]: do not check truth value of flex arrays -Peff ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] read_and_strip_branch: fix typo'd address-of operator 2015-01-28 17:53 [PATCH 0/2] silence clang-3.6 warnings Jeff King @ 2015-01-28 17:57 ` Jeff King 2015-01-28 20:42 ` Junio C Hamano 2015-01-28 17:58 ` [PATCH 2/2] do not check truth value of flex arrays Jeff King 1 sibling, 1 reply; 6+ messages in thread From: Jeff King @ 2015-01-28 17:57 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy When we are chomping newlines from the end of a strbuf, we must check "sb.len != 0" before accessing "sb.buf[sb.len - 1]". However, this code mistakenly checks "&sb.len", which is always true (it is a part of an auto struct, so the address is always non-zero). This could lead to us accessing memory outside the strbuf when we read an empty file. Signed-off-by: Jeff King <peff@peff.net> --- This dates back to 8b87cfd (wt-status: move strbuf into read_and_strip_branch(), 2013-03-16), so it is not a bug that needs addressed during the -rc period. This is the most minimal fix, but I kind of wonder if it should just be using strbuf_rtrim (or even strbuf_trim) in the first place. wt-status.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wt-status.c b/wt-status.c index b54eac5..29666d0 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1140,7 +1140,7 @@ static char *read_and_strip_branch(const char *path) if (strbuf_read_file(&sb, git_path("%s", path), 0) <= 0) goto got_nothing; - while (&sb.len && sb.buf[sb.len - 1] == '\n') + while (sb.len && sb.buf[sb.len - 1] == '\n') strbuf_setlen(&sb, sb.len - 1); if (!sb.len) goto got_nothing; -- 2.3.0.rc1.287.g761fd19 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] read_and_strip_branch: fix typo'd address-of operator 2015-01-28 17:57 ` [PATCH 1/2] read_and_strip_branch: fix typo'd address-of operator Jeff King @ 2015-01-28 20:42 ` Junio C Hamano 2015-01-28 22:57 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2015-01-28 20:42 UTC (permalink / raw) To: Jeff King; +Cc: git, Nguyễn Thái Ngọc Duy Jeff King <peff@peff.net> writes: > When we are chomping newlines from the end of a strbuf, we > must check "sb.len != 0" before accessing "sb.buf[sb.len - 1]". > However, this code mistakenly checks "&sb.len", which is > always true (it is a part of an auto struct, so the address > is always non-zero). This could lead to us accessing memory > outside the strbuf when we read an empty file. > > Signed-off-by: Jeff King <peff@peff.net> > --- > This dates back to 8b87cfd (wt-status: move strbuf into > read_and_strip_branch(), 2013-03-16), so it is not a bug that needs > addressed during the -rc period. > > This is the most minimal fix, but I kind of wonder if it should just be > using strbuf_rtrim (or even strbuf_trim) in the first place. Yeah. Or strbuf_chomp(), which does not exist ;-) It is tempting to apply this directly to maint and merge up immediately, as there is no way this 1-byte change will break things (of course that is not necessarily true for random 1-byte changes, though). It sometimes gets really hard to resist that temptation during the pre-release freeze period. > wt-status.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/wt-status.c b/wt-status.c > index b54eac5..29666d0 100644 > --- a/wt-status.c > +++ b/wt-status.c > @@ -1140,7 +1140,7 @@ static char *read_and_strip_branch(const char *path) > if (strbuf_read_file(&sb, git_path("%s", path), 0) <= 0) > goto got_nothing; > > - while (&sb.len && sb.buf[sb.len - 1] == '\n') > + while (sb.len && sb.buf[sb.len - 1] == '\n') > strbuf_setlen(&sb, sb.len - 1); > if (!sb.len) > goto got_nothing; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] read_and_strip_branch: fix typo'd address-of operator 2015-01-28 20:42 ` Junio C Hamano @ 2015-01-28 22:57 ` Jeff King 2015-01-29 6:22 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2015-01-28 22:57 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Nguyễn Thái Ngọc Duy On Wed, Jan 28, 2015 at 12:42:26PM -0800, Junio C Hamano wrote: > > This is the most minimal fix, but I kind of wonder if it should just be > > using strbuf_rtrim (or even strbuf_trim) in the first place. > > Yeah. Or strbuf_chomp(), which does not exist ;-) This is not the first time I've seen this chomp/trim distinction come up. However, the thing that has prevented me from writing strbuf_chomp is that the trim is almost always a more reasonable choice. Take this instance. We are opening and reading a whole file. Surely we need to drop the final newline, which is not interesting. But we are not just doing that; we are dropping _all_ trailing newlines. So "foo\n\n" becomes "foo". But "foo\n \n" does not. That doesn't make much sense. IOW, I would venture to say that chomping like this falls into one of two categories: 1. You want to clean up any extraneous cruft. Multiple lines, extra whitespace, etc. 2. You want to read one line, but don't want the trailing newline. And strbuf_getline already handles case (2). End mini-rant. :) > It is tempting to apply this directly to maint and merge up > immediately, as there is no way this 1-byte change will break things > (of course that is not necessarily true for random 1-byte changes, > though). > > It sometimes gets really hard to resist that temptation during the > pre-release freeze period. That's part of why I did the simplest fix instead of strbuf_rtrim. To tempt you. :) -Peff ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] read_and_strip_branch: fix typo'd address-of operator 2015-01-28 22:57 ` Jeff King @ 2015-01-29 6:22 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2015-01-29 6:22 UTC (permalink / raw) To: Jeff King; +Cc: git, Nguyễn Thái Ngọc Duy Jeff King <peff@peff.net> writes: > This is not the first time I've seen this chomp/trim distinction come > up. However, the thing that has prevented me from writing strbuf_chomp > is that the trim is almost always a more reasonable choice. > ... > End mini-rant. :) Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] do not check truth value of flex arrays 2015-01-28 17:53 [PATCH 0/2] silence clang-3.6 warnings Jeff King 2015-01-28 17:57 ` [PATCH 1/2] read_and_strip_branch: fix typo'd address-of operator Jeff King @ 2015-01-28 17:58 ` Jeff King 1 sibling, 0 replies; 6+ messages in thread From: Jeff King @ 2015-01-28 17:58 UTC (permalink / raw) To: git; +Cc: Junio C Hamano There is no point in checking "!ref->name" when ref is a "struct ref". The name field is a flex-array, and there always has a non-zero address. This is almost certainly not hurting anything, but it does cause clang-3.6 to complain. Signed-off-by: Jeff King <peff@peff.net> --- Note that even if "ref" is NULL, this is still going to be "0+offsetof(name)". So short of unsigned pointer wrap-around, I do not think this check can ever do anything. remote-curl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/remote-curl.c b/remote-curl.c index dd63bc2..515ac9b 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -760,7 +760,7 @@ static int fetch_git(struct discovery *heads, for (i = 0; i < nr_heads; i++) { struct ref *ref = to_fetch[i]; - if (!ref->name || !*ref->name) + if (!*ref->name) die("cannot fetch by sha1 over smart http"); packet_buf_write(&preamble, "%s %s\n", sha1_to_hex(ref->old_sha1), ref->name); -- 2.3.0.rc1.287.g761fd19 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-01-29 6:24 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-28 17:53 [PATCH 0/2] silence clang-3.6 warnings Jeff King 2015-01-28 17:57 ` [PATCH 1/2] read_and_strip_branch: fix typo'd address-of operator Jeff King 2015-01-28 20:42 ` Junio C Hamano 2015-01-28 22:57 ` Jeff King 2015-01-29 6:22 ` Junio C Hamano 2015-01-28 17:58 ` [PATCH 2/2] do not check truth value of flex arrays Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.