Miscellaneous fixes from static analysis

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

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

---
 notes-utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2975dcd..4aa7023 100644
@@ -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

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

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

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

Yes?

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

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

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

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

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) {
          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.

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

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

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

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

Assuming that.

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

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

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

---
 utf8.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 0d20e0a..24c3c5c 100644
@@ -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

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.

---
 utf8.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 24c3c5c..a831d50 100644
@@ -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

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.

---
 builtin/mv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 21c46d1..7e26eb5 100644
@@ -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

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.

---
 streaming.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index d7c9f32..8a7135d 100644
@@ -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

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.

---
 streaming.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index d7c9f32..2ff036a 100644
@@ -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