patch, 5 partsMiscellaneous 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(-)

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(-)

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

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?

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

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

Jeff King <peff@peff.net> writes:

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.

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

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

Jeff King <peff@peff.net> writes:

    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.

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(-)

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(-)

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(-)

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(-)

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

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.

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

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(-)