[PATCH 0/5] Miscellaneous fixes from static analysis

[PATCH 0/5] Miscellaneous fixes from static analysis

[John Keeping]

The first two of these fix real bugs, the rest just clean up some of the
less obviously "not actually a problem" issues identified by Clang's
static analyzer [1] and stack[2].

Stack is interesting in that it is designed to detect potentially
undesirable optimizations where undefined behaviour may be being invoked
unwittingly.  It only detected two error's in git.git, the first of
which is fixed by the final patch.  The second it describes as:

	bug: anti-algebra
	model: |
	  %11 = icmp ult i8* %extra_args, %7, !dbg !342
	  -->  %10 = icmp slt i64 %9, 0, !dbg !342
	  ************************************************************
	  %extra_args <u ((sext i32 %buflen to i64) + %extra_args)<nsw>
	  -->  (-1 * (sext i32 %buflen to i64)) <s 0
	stack: 
	  - daemon.c:522:0
	ncore: 1
	core: 
	  - daemon.c:520:0
	    - pointer overflow

which shows that Clang has converted (simplifying from daemon.c:520):

	char *end = extra_args + buflen;
	if (extra_args < end)

into:

	if (buflen < 0)

This doesn't look like it can ever be subject to pointer overflow, so I
have not considered the churn worth it here.


[1] http://clang-analyzer.llvm.org/
[2] https://github.com/xiw/stack

John Keeping (5):
  notes-utils: handle boolean notes.rewritemode correctly
  utf8: fix iconv error detection
  utf8: use correct type for values in interval table
  builtin/mv: don't use memory after free
  streaming: simplify attaching a filter

 builtin/mv.c  | 3 ++-
 notes-utils.c | 2 +-
 streaming.c   | 5 +----
 utf8.c        | 6 +++---
 4 files changed, 7 insertions(+), 9 deletions(-)

-- 
1.9.rc0.187.g6292fff

[PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly

[John Keeping]

If we carry on after outputting config_error_nonbool then we're
guaranteed to dereference a null pointer.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 notes-utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/notes-utils.c b/notes-utils.c
index 2975dcd..4aa7023 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -75,7 +75,7 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)
 		return 0;
 	} else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
 		if (!v)
-			config_error_nonbool(k);
+			return config_error_nonbool(k);
 		c->combine = parse_combine_notes_fn(v);
 		if (!c->combine) {
 			error(_("Bad notes.rewriteMode value: '%s'"), v);
-- 
1.9.rc0.187.g6292fff

Re: [PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly

[David Kastrup]

John Keeping <john@keeping.me.uk> writes:

> If we carry on after outputting config_error_nonbool then we're
> guaranteed to dereference a null pointer.

Not really relevant to this patch, but looking at the output of

git grep config_error_nonbool

seems like a serious amount of ridiculousness going on.  The header
shows

cache.h:extern int config_error_nonbool(const char *);
cache.h:#define config_error_nonbool(s) (config_error_nonbool(s), -1)

and the implementation

config.c:#undef config_error_nonbool
config.c:int config_error_nonbool(const char *var)

Presumably this was done so that the uses of config_error_nonbool can be
recognized as returning -1 unconditionally.

But is that worth the obfuscation?  Why not let config_error_nonbool
return -1 in the first place?  It does not appear like any caller would
call the function rather than the macro, so why declare the function as
returning an int at all?  And why give it the same name as the macro
(risking human/computer confusion and requiring an explicit #undef for
the definition or was that declaration?) instead of
config_error_nonbool_internal or whatever else?

-- 
David Kastrup

Re: [PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly

[Jeff King]

On Sun, Feb 16, 2014 at 05:22:45PM +0100, David Kastrup wrote:

> Not really relevant to this patch, but looking at the output of
> 
> git grep config_error_nonbool
> 
> seems like a serious amount of ridiculousness going on.  The header
> shows
> 
> cache.h:extern int config_error_nonbool(const char *);
> cache.h:#define config_error_nonbool(s) (config_error_nonbool(s), -1)
> 
> and the implementation
> 
> config.c:#undef config_error_nonbool
> config.c:int config_error_nonbool(const char *var)

You could always look in the commit history:

  $ git log -S'#define config_error_nonbool' cache.h

or search the mailing list:

  http://thread.gmane.org/gmane.comp.version-control.git/211505

> Presumably this was done so that the uses of config_error_nonbool can be
> recognized as returning -1 unconditionally.

Yes, it helps prevent false positives in gcc's flow analysis
(specifically, -Wuninitialized warnings).

> But is that worth the obfuscation?

Yes?

> Why not let config_error_nonbool return -1 in the first place?

It originally did, but callers do not get to see the static return, so
they miss some analysis opportunities.

> It does not appear like any caller would call the function rather than
> the macro, so why declare the function as returning an int at all?

Because we don't use these macros on non-gnu compilers; they actually
see the real function return.

> And why give it the same name as the macro (risking human/computer
> confusion and requiring an explicit #undef for the definition or was
> that declaration?) instead of config_error_nonbool_internal or
> whatever else?

This particular case is simple, but see error() for another use of the
same technique which requires variadic macros, which are not available
everywhere. Callers want to say just "error(...)", so we have to name
the macro that. If we call the matching function "error_internal", then
non-gnu compilers would need a macro to convert "error(...)" to
"error_internal(...)". But they can't do so, because it would be a
variadic macro.

So you could adjust config_error_nonbool(), but not error(). But that
introduces its own confusion, because the technique is not applied
consistently.

I agree the #define is ugly. But it's confined to a small area, and it
does solve an actual problem.

-Peff

Re: [PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly

[David Kastrup]

Jeff King <peff@peff.net> writes:

> On Sun, Feb 16, 2014 at 05:22:45PM +0100, David Kastrup wrote:
>> 
>> config.c:#undef config_error_nonbool
>> config.c:int config_error_nonbool(const char *var)
>
> You could always look in the commit history:
>
>   $ git log -S'#define config_error_nonbool' cache.h
>
> or search the mailing list:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/211505
>
>> Presumably this was done so that the uses of config_error_nonbool can be
>> recognized as returning -1 unconditionally.
>
> Yes, it helps prevent false positives in gcc's flow analysis
> (specifically, -Wuninitialized warnings).
>
>> But is that worth the obfuscation?
>
> Yes?

gcc's flow analysis works with the same data as humans reading the
code.  If there is no information content in the function call, it makes
more sense to either making it void.

One can always explicitly write

  (config_error_nonbool("panic-when-assailed"), -1)

for shortcircuit use inside of an expression even when
config_error_nonbool is a function returning a void expression: comma
operators do not try type coercion on their first argument.

Shrug.  This one has likely been discussed to death already.  Sometimes
it's more convenient to avoid getting a question asked in the first
place rather than having a stock answer for it.

-- 
David Kastrup

Re: [PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly

[Jeff King]

On Tue, Feb 18, 2014 at 09:41:51AM +0100, David Kastrup wrote:

> gcc's flow analysis works with the same data as humans reading the
> code.  If there is no information content in the function call, it makes
> more sense to either making it void.

The point of error() returning a constant -1 is to use this idiom:

  if (something_failed)
          return error("this will get printed, and we get a -1 return");

>From a code perspective it's pointless. You could "just" write:

  if (something_failed) {
          error(...);
          return -1;
  }

which is equivalent. But the point is that the former is shorter and a
little more readable, assuming you are familiar with the idiom.

> One can always explicitly write
> 
>   (config_error_nonbool("panic-when-assailed"), -1)

Yes, but again, the point is readability. Doing that at each callsite is
ugly and annoying.

> Shrug.  This one has likely been discussed to death already.  Sometimes
> it's more convenient to avoid getting a question asked in the first
> place rather than having a stock answer for it.

You are the first person to ask about it, so there is no stock answer.
However, everything I told you was in the commit messages and the list
archive already. We can also avoid questions being asked by using those
tools.

-Peff

Re: [PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly

[David Kastrup]

Jeff King <peff@peff.net> writes:

> On Tue, Feb 18, 2014 at 09:41:51AM +0100, David Kastrup wrote:
>
>> gcc's flow analysis works with the same data as humans reading the
>> code.  If there is no information content in the function call, it makes
>> more sense to either making it void.
>
> The point of error() returning a constant -1 is to use this idiom:
>
>   if (something_failed)
>           return error("this will get printed, and we get a -1 return");

    if (something_failed)
        return error("this will get printed"), -1;

Not much of an idiom, no insider logic hidden to both compiler and
reader.

>>From a code perspective it's pointless. You could "just" write:
>
>   if (something_failed) {
>           error(...);
>           return -1;
>   }
>
> which is equivalent. But the point is that the former is shorter and a
> little more readable, assuming you are familiar with the idiom.

Assuming that.

>> One can always explicitly write
>> 
>>   (config_error_nonbool("panic-when-assailed"), -1)
>
> Yes, but again, the point is readability. Doing that at each callsite is
> ugly and annoying.

I consider insider semantics ugly and annoying.  To each his own.

> You are the first person to ask about it, so there is no stock answer.
> However, everything I told you was in the commit messages and the list
> archive already. We can also avoid questions being asked by using
> those tools.

It's further raising the barriers for contributors if they are expected
to have studied the last few years in the mailing archive.  And the
skills needed for that kind of study are mostly unrelated to good
programming skills.

So I am less than convinced that this is among the coding practices that
can be expected to attract/scare away potential contributors in
proportion to their expected capability of advancing/hindering the
project.

It's not me running the shop, so it's nothing more than an opinion.

-- 
David Kastrup

[PATCH 2/5] utf8: fix iconv error detection

[John Keeping]

iconv(3) returns "(size_t) -1" on error.  Make sure that we cast the
"-1" properly when checking for this.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 utf8.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utf8.c b/utf8.c
index 0d20e0a..24c3c5c 100644
--- a/utf8.c
+++ b/utf8.c
@@ -529,7 +529,7 @@ char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv, int *outs
 	while (1) {
 		size_t cnt = iconv(conv, &cp, &insz, &outpos, &outsz);
 
-		if (cnt == -1) {
+		if (cnt == (size_t) -1) {
 			size_t sofar;
 			if (errno != E2BIG) {
 				free(out);
-- 
1.9.rc0.187.g6292fff

[PATCH 3/5] utf8: use correct type for values in interval table

[John Keeping]

We treat these as unsigned everywhere and compare against unsigned
values, so declare them using the typedef we already have for this.

While we're here, fix the indentation as well.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 utf8.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/utf8.c b/utf8.c
index 24c3c5c..a831d50 100644
--- a/utf8.c
+++ b/utf8.c
@@ -5,8 +5,8 @@
 /* This code is originally from http://www.cl.cam.ac.uk/~mgk25/ucs/ */
 
 struct interval {
-  int first;
-  int last;
+	ucs_char_t first;
+	ucs_char_t last;
 };
 
 size_t display_mode_esc_sequence_len(const char *s)
-- 
1.9.rc0.187.g6292fff

[PATCH 4/5] builtin/mv: don't use memory after free

[John Keeping]

If 'src' already ends with a slash, then add_slash() will just return
it, meaning that 'free(src_with_slash)' is actually 'free(src)'.  Since
we use 'src' later, this will result in use-after-free.

In fact, this cannot happen because 'src' comes from
internal_copy_pathspec() without the KEEP_TRAILING_SLASH flag, so any
trailing '/' will have been stripped; but static analysis tools are not
clever enough to realise this and so warn that 'src' could be used after
having been free'd.  Fix this by checking that 'src_w_slash' is indeed
newly allocated memory.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 builtin/mv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index 21c46d1..7e26eb5 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -162,7 +162,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 					if (strncmp(path, src_w_slash, len_w_slash))
 						break;
 				}
-				free((char *)src_w_slash);
+				if (src_w_slash != src)
+					free((char *)src_w_slash);
 
 				if (last - first < 1)
 					bad = _("source directory is empty");
-- 
1.9.rc0.187.g6292fff

[PATCH 5/5] streaming: simplify attaching a filter

[John Keeping]

We are guaranteed that 'nst' is non-null because it is allocated with
xmalloc(), and in fact we rely on this three lines later by
unconditionally dereferencing it.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 streaming.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/streaming.c b/streaming.c
index d7c9f32..8a7135d 100644
--- a/streaming.c
+++ b/streaming.c
@@ -151,10 +151,7 @@ struct git_istream *open_istream(const unsigned char *sha1,
 	}
 	if (filter) {
 		/* Add "&& !is_null_stream_filter(filter)" for performance */
-		struct git_istream *nst = attach_stream_filter(st, filter);
-		if (!nst)
-			close_istream(st);
-		st = nst;
+		st = attach_stream_filter(st, filter);
 	}
 
 	*size = st->size;
-- 
1.9.rc0.187.g6292fff

Re: [PATCH 5/5] streaming: simplify attaching a filter

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> We are guaranteed that 'nst' is non-null because it is allocated with
> xmalloc(), and in fact we rely on this three lines later by
> unconditionally dereferencing it.

The intent of the original code is for attach_stream_filter() to
detect an error condition and return NULL, in which case it closes
the istream it allocated and signal error to the caller, I think,
and falling thru to use st->anything and return st when that happens
is *not* a guarantee that a-s-f will not detect an error ever, but
rather is a bug in the error codepath.

>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  streaming.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/streaming.c b/streaming.c
> index d7c9f32..8a7135d 100644
> --- a/streaming.c
> +++ b/streaming.c
> @@ -151,10 +151,7 @@ struct git_istream *open_istream(const unsigned char *sha1,
>  	}
>  	if (filter) {
>  		/* Add "&& !is_null_stream_filter(filter)" for performance */
> -		struct git_istream *nst = attach_stream_filter(st, filter);
> -		if (!nst)
> -			close_istream(st);
> -		st = nst;
> +		st = attach_stream_filter(st, filter);
>  	}
>  
>  	*size = st->size;

Re: [PATCH 5/5] streaming: simplify attaching a filter

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> John Keeping <john@keeping.me.uk> writes:
>
>> We are guaranteed that 'nst' is non-null because it is allocated with
>> xmalloc(), and in fact we rely on this three lines later by
>> unconditionally dereferencing it.
>
> The intent of the original code is for attach_stream_filter() to
> detect an error condition and return NULL, in which case it closes
> the istream it allocated and signal error to the caller, I think,
> and falling thru to use st->anything and return st when that happens
> is *not* a guarantee that a-s-f will not detect an error ever, but
> rather is a bug in the error codepath.

In other words, something like this instead.

-- >8 --
Subject: [PATCH] open_istream(): do not dereference NULL in the error case

When stream-filter cannot be attached, it is expected to return NULL,
and we should close the stream we opened and signal an error by
returning NULL ourselves from this function.

However, we attempted to dereference that NULL pointer between the
point we detected the error and returned from the function.

Brought-to-attention-by: John Keeping <john@keeping.me.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 streaming.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/streaming.c b/streaming.c
index d7c9f32..2ff036a 100644
--- a/streaming.c
+++ b/streaming.c
@@ -152,8 +152,10 @@ struct git_istream *open_istream(const unsigned char *sha1,
 	if (filter) {
 		/* Add "&& !is_null_stream_filter(filter)" for performance */
 		struct git_istream *nst = attach_stream_filter(st, filter);
-		if (!nst)
+		if (!nst) {
 			close_istream(st);
+			return NULL;
+		}
 		st = nst;
 	}
 
-- 
1.9.0-279-gdc9e3eb