patch, 3 partsFix code issues spotted by clang

This series fixes some of the code issues raised by clang. This leaves the following warnings that I haven't addressed:

    revision.c:766:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967279 to 134217711
          [-Wconstant-conversion]
                    p->item->object.flags &= ~TMP_MARK;
                                          ^  ~~~~~~~~~
    revision.c:768:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967279 to 134217711
          [-Wconstant-conversion]
                    p->item->object.flags &= ~TMP_MARK;
                                          ^  ~~~~~~~~~
    revision.c:1875:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967279 to 134217711
          [-Wconstant-conversion]
                    p->item->object.flags &= ~TMP_MARK;
                                          ^  ~~~~~~~~~
    revision.c:2202:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967158 to 134217590
          [-Wconstant-conversion]
                            commit->object.flags &= ~(ADDED | SEEN | SHOWN);
                                                 ^  ~~~~~~~~~~~~~~~~~~~~~~~
    upload-pack.c:115:12: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967293 to 134217725
          [-Wconstant-conversion]
                    o->flags &= ~UNINTERESTING;
                             ^  ~~~~~~~~~~~~~~
    upload-pack.c:689:19: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294705151 to 133955583
          [-Wconstant-conversion]
                                    object->flags &= ~CLIENT_SHALLOW;
                                                  ^  ~~~~~~~~~~~~~~~
    builtin/checkout.c:676:16: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967293 to 134217725
          [-Wconstant-conversion]
            object->flags &= ~UNINTERESTING;
                          ^  ~~~~~~~~~~~~~~
    builtin/reflog.c:173:32: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294965247 to 134215679
          [-Wconstant-conversion]
                    found.objects[i].item->flags &= ~STUDYING;
                                                 ^  ~~~~~~~~~
    builtin/reflog.c:232:31: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294963199 to 134213631
          [-Wconstant-conversion]
                    pending->item->object.flags &= ~REACHABLE;
                                                ^  ~~~~~~~~~~
    bisect.c:66:24: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294901759 to 134152191
          [-Wconstant-conversion]
                    commit->object.flags &= ~COUNTED;
                                         ^  ~~~~~~~~
    
Ævar Arnfjörð Bjarmason (3):
  apply: get rid of useless x < 0 comparison on a size_t type
  diff/apply: cast variable in call to free()
  grep: get rid of useless x < 0 comparison on an enum member

 builtin/apply.c |    3 ---
 builtin/diff.c  |    2 +-
 grep.c          |    2 +-
 submodule.c     |    2 +-
 4 files changed, 3 insertions(+), 6 deletions(-)

According to the C standard size_t is always unsigned, therefore the comparison "n1 < 0 || n2 < 0" when n1 and n2 are size_t will always be false.

This was raised by clang 2.9 which throws this warning when compiling apply.c:

    builtin/apply.c:253:9: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
            if (n1 < 0 || n2 < 0)
                ~~ ^ ~
    builtin/apply.c:253:19: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
            if (n1 < 0 || n2 < 0)
                          ~~ ^ ~

This check was originally added in v1.6.5-rc0~53^2 by Giuseppe Bilotta while adding an option to git-apply to ignore whitespace differences.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/apply.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I agree that these quantities can never be negative, so I'll apply the patch as is.

But I have this suspicion that this was a rather sloppy defensive check to protect this codepath against potential breakage in another codepath (most likely update_pre_post_images() touched by the same patch) that adjusts image->line[].len the caller of this function uses to feed these two parameters. Giuseppe may have been not confident enough that the code added to that function ensures not to undershoot when it reduces "len", or something.

Giuseppe, can you explain what is going on?

Thanks

On Sun, Nov 6, 2011 at 7:35 PM, Junio C Hamano <gitster@pobox.com> wrote:

I agree too. In fact I spotted this some time ago when I started compiling git with Clang but never found the time to look into it.

No, I can't, so I guess the sloppy coding is the right motivation. I remember working this patch from a rather older submission that was never followed-up to, so my guess is that I just forgot to clean it up properly, and during review the focus was obviously on other aspects of the submission too. Also, having a look at the current caller of the function, I don't see how the check would even be needed.

Both of these free() calls are freeing a "const unsigned char (*)[20]" type while free() expects a "void *". This results in the following warning under clang 2.9:

    builtin/diff.c:185:7: warning: passing 'const unsigned char (*)[20]' to parameter of type 'void *' discards qualifiers
            free(parent);
                 ^~~~~~

    submodule.c:394:7: warning: passing 'const unsigned char (*)[20]' to parameter of type 'void *' discards qualifiers
            free(parents);
                 ^~~~~~~

This free()-ing without a cast was added by Jim Meyering to builtin/diff.c in v1.7.6-rc3~4 and later by Fredrik Gustafsson in submodule.c in v1.7.7-rc1~25^2.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/diff.c |    2 +-
 submodule.c    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Remove an "p->field < 0" comparison in grep.c that'll always be false. In this case "p" is a "grep_pat" where "field" is defined as:

	enum grep_header_field field;

And grep_header_field is in turn defined as:

    enum grep_header_field {
    	GREP_HEADER_AUTHOR = 0,
    	GREP_HEADER_COMMITTER
    };

Meaning that this comparison will always be false. This was spotted by clang 2.9 which produced the following warning while compiling grep.c:

    grep.c:330:16: warning: comparison of unsigned enum expression < 0 is always false [-Wtautological-compare]
                    if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
                        ~~~~~~~~ ^ ~

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

The underlying integer type is implementation-defined, and can be any signed or unsigned integer type that can represent all enumerations.

Andreas.

On Sun, Nov 6, 2011 at 16:03, Andreas Schwab <schwab@linux-m68k.org> wrote:

Yes, but as far as I can tell since we've done "= 0" there that doesn't apply to us. To quote the ANSI C Standard (ANSI X3J11/88-090):

    3.5.2.2 Enumeration specifiers

    Syntax

              enum-specifier:
                      enum  identifier<opt> { enumerator-list }
                      enum  identifier

              enumerator-list:
                      enumerator
                      enumerator-list , enumerator

              enumerator:
                      enumeration-constant
                      enumeration-constant = constant-expression

    Constraints

       The expression that defines the value of an enumeration constant
    shall be an integral constant expression that has a value
    representable as an int.

    Semantics

       The identifiers in an enumerator list are declared as constants
    that have type int and may appear wherever such are permitted./52/ An
    enumerator with = defines its enumeration constant as the value of the
    constant expression.  If the first enumerator has no = , the value of
    its enumeration constant is 0.  Each subsequent enumerator with no =
    defines its enumeration constant as the value of the constant
    expression obtained by adding 1 to the value of the previous
    enumeration constant.  (A combination of both forms of enumerators may
    produce enumeration constants with values that duplicate other values
    in the same enumeration.) The enumerators of an enumeration are also
    known as its members.

       Each enumerated type shall be compatible with an integer type; the
    choice of type is implementation-defined.

    Example

             enum hue { chartreuse, burgundy, claret=20, winedark };
             /*...*/
             enum hue col, *cp;
             /*...*/
             col = claret;
             cp = &col;
             /*...*/
             /*...*/ (*cp != burgundy) /*...*/

    makes hue the tag of an enumeration, and then declares col as an
    object that has that type and cp as a pointer to an object that has
    that type.  The enumerated values are in the set {0, 1, 20, 21}.

I.e. we'll always have GREP_HEADER_AUTHOR = 0 and GREP_HEADER_COMMITTER = 1, we'll never have GREP_HEADER_AUTHOR = and GREP_HEADER_COMMITTER = <some int>.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

That is irrelevant. You can always assign -1 to an object of enumerated type and the implicit conversion to the underlying type is fully defined, no matter what type the compiler choses.

Andreas.

On Mon, Nov 07, 2011 at 02:12:28PM +0100, Andreas Schwab wrote:

Yes, but now you are getting into implementation-defined behavior, which is also something to avoid. IOW, I don't think it's wrong for a static analyzer to complain about:

  if (enum_with_only_positive_values < 0)

just because you could say:

  enum_with_only_positive_values = -1;

and it would work on _some_ platforms. That same static analyzer should be complaining about the second line, which is where the real potential bug is. If you want "-1" as an enumerated value, then add it to your enum definition and the compiler will do the right thing.

It is perfectly fine to do:

     enum foo { a = -1, b, c };
     ...
     if (foo < 0)
        ...

and the static analyzer should not complain about the conditional there.

-Peff

Jeff King <peff@peff.net> writes:

The whole point of the statement is a sanity check to uncover bugs. If you remove the first condition you completely ruin its point.

Andreas.

On Mon, Nov 07, 2011 at 07:24:05PM +0100, Andreas Schwab wrote:

I'm somewhat dubious of the value of a bug-check that may or may not actually kick in depending on your compiler's choice of enum representation, and whose bugs are generally easier to check via static analysis (i.e., making sure the enum value is one of the enumerated values when it is initialized or assigned).

Yes, static analysis can miss some bugs (like passing the address of the enum through a void pointer (e.g., when memset'ing a struct)). But couldn't it just as easily be out of range in the other direction?

It seems like the bug trying to be caught is probably something like:

  enum foo v = function_which_returns_value_or_negative_error();
  if (v < 0)
     die("BUG");

But in that case the bug is on the first line, and it is easily caught by static analysis, no?

-Peff

On Mon, Nov 07, 2011 at 01:34:02PM -0500, Jeff King wrote:

Oops, I just looked at the actual conditional, and it is indeed range-checking the whole enum. So if you are going to do that at all, then yes, checking both sides is sensible, and that is what the original conditional does. Sorry for my confusion.

I do still question the value of the check at all, though, given that static analysis can find those bugs.

-Peff

On Mon, Nov 7, 2011 at 19:55, Jeff King <peff@peff.net> wrote:

Agreed, which is why I submitted this patch.

Also if this is the sort of thing we'd like to guard against we should be discussing it more generally. We have a bunch of occurances where we'd probably break down if someone manually assigned -1 to an enum variable:

    $ git grep -E -A1 'enum.*\{' | grep -B1 '=.*0' | grep enum
    builtin/branch.c:enum color_branch {
    builtin/branch.c:static enum merge_filter {
    builtin/fetch-pack.c:enum ack_type {
    builtin/fetch.c:enum {
    builtin/grep.c: enum {
    builtin/mv.c:   enum update_mode { BOTH = 0, WORKING_DIRECTORY,
INDEX } *modes;
    builtin/remote.c:enum {
    builtin/remote.c:       enum {
    cache.h:enum rebase_setup_type {
    cache.h:enum push_default_type {
    cache.h:enum object_creation_mode {
    cache.h:enum sharedrepo {
    cache.h:enum date_mode {
    cache.h:        enum {
    convert.h:enum safe_crlf {
    convert.h:enum auto_crlf {
    diff.h:enum diff_words_type {
    diff.h:enum color_diff {
    dir.c:enum exist_status {
    dir.h:  enum {
    grep.h:enum grep_header_field {
    http-push.c:enum dav_header_flag {
    imap-send.c:enum CAPABILITY {
    log-tree.c:enum decoration_type {
    merge-recursive.c:enum rename_type {
    merge-recursive.h:      enum {
    notes-merge.h:  enum {
    remote.h:enum match_refs_flags {
    rerere.c:       enum {
    unpack-trees.h:enum unpack_trees_error_types {
    wt-status.h:enum color_wt_status {

Jeff King <peff@peff.net> writes:

Then you should remove the whole statement.

Andreas.

Hi,

I imagine the following would be less controversial.

-- >8 --
From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Subject: grep: get rid of bounds check on "enum grep_header_field" value

The grep_header_field enum is defined as:

    enum grep_header_field {
    	GREP_HEADER_AUTHOR = 0,
    	GREP_HEADER_COMMITTER
    };

Git's grep code sanity-checks the value of p->field before using it in order to avoid reading and writing past the end of an array. Unfortunately that test trips up misguided static analyzers like "gcc -Wtype-limits" that do not recognize that C allows this enum to be a signed integer type and an "x < 0" comparison really could fire if someone passed in an uninitialized value.

Let's just drop the check. Luckily git always does initialize p->field correctly, and if it were to stop doing so, then hopefully running the test suite with valgrind would catch it.

Noticed with clang -Wtautological-compare.

[jn: drop the GREP_HEADER_FIELD_MAX <= p->field check, too,
 for symmetry]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 grep.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

Jonathan Nieder <jrnieder@gmail.com> writes:

Umm, why is this removal of defensive programming practice an improvement? This part is mostly my code and because I know what I wrote is almost always perfect, so I do not think there is any real harm done, but still...

Junio C Hamano wrote:

Checking "p->filed < 0" makes static analyzers complain. There's no clean way I know of[*] to get them to shut up and keep the check. The fundamental question is whether the -Wtype-limits check is worth it or not:

 - on one hand, it catches real bugs, such as the mistaken check for
   negative size_t Ævar mentioned;

 - on the other hand, it trips for cases like this in which the type
   only happened to be unsigned on the build platform.  I consider
   that a gcc bug (and apparently clang shares it) --- see
   <http://bugs.debian.org/615525>.

So, the purpose of this patch was to work around this common bug in static analyzers.

Checking "GREP_HEADER_FIELD_MAX <= p->field" without checking for "p->field < 0", like the original patch did, would be only checking half of what it intends to and not add any real guarantees. And more importantly, it would be distracting to people like me and Andreas who would wonder "why doesn't this check p->field < 0, too?".

I guess the commit message should have said

	grep: remove a defensive check to work around common static analyzer bug

Heh.

[*] There are plenty of cryptic, hackish ways possible, though. :)

	if ((size_t) p->field >= GREP_HEADER_FIELD_MAX)
		die(...);

Jonathan Nieder <jrnieder@gmail.com> writes:

I fail to see how it could be even considered a work around.

If you do not use static analyzers, you do not have to do such a change, and the resulting code would (not the "negative" side, but the "positive" side) catch real bugs when somebody screwes up and stuffs a bogus oob value in p->field.

With the removal of the check, you _have_ to rely on static analyzers to do the _right thing_, but if you have static analyzers that do the right thing, you do not have to have such a workaround to begin with.

Junio C Hamano wrote:

No, automatic static analyzers will not help you avoid using an uninitialized enum value here (well, I guess they could, but I don't believe gcc or clang is nearly smart enough for that). The only replacement for the check is (1) humans and (2) valgrind.

In this particular case, as you mentioned before, (1) seems to be quite sufficient.

On Sun, Nov 6, 2011 at 13:06, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Out of curiosity I also compiled git on "Sun Studio 12 Update 1" on ee6dfb2d83ba1b057943e705f707fa27e34e47f9 with this patch series applied and it emits the following warnings, quoted below along with my comments:

    "pack-write.c", line 76: warning: name redefined by pragma
redefine_extname declared static: tmpfile

Presumably this means that we've imported the function tmpfile() and Sun's CC doesn't like that we're creating a variable with that name.

    "read-cache.c", line 761: warning: statement not reached

SunCC's brain is melting on this particularly clever piece of code:

    	goto inside;
    	for (;;) {
    		if (!c)
    			return 1;
    		if (is_dir_sep(c)) {
    inside:

    "sha1_file.c", line 2453: warning: name redefined by pragma
redefine_extname declared static: tmpfile

same tempfile issue.

    "compat/../git-compat-util.h", line 4: warning: macro redefined:
_FILE_OFFSET_BITS
    "compat/../git-compat-util.h", line 4: warning: macro redefined:
_FILE_OFFSET_BITS

Seems we're clashing with this:

    $ grep -r '#define.*_FILE_OFFSET_BITS' /usr/include/
    /usr/include/sys/feature_tests.h:#define   _FILE_OFFSET_BITS   64
    /usr/include/sys/feature_tests.h:#define   _FILE_OFFSET_BITS   32

    "xdiff/xutils.c", line 194: warning: statement not reached

This was added in b97e911643341cb31e6b97029b9ffd96fc675b1d as a workaround on systems using glibc, should this even be run on Sun libc? Martin?

Another clever bit of code blowing SunCC's brain:

	if (flags & XDF_IGNORE_WHITESPACE) {
		goto skip_ws;
		while (i1 < s1 && i2 < s2) {
			if (l1[i1++] != l2[i2++])
				return 0;
		skip_ws:
			while (i1 < s1 && XDL_ISSPACE(l1[i1]))
				i1++;
			while (i2 < s2 && XDL_ISSPACE(l2[i2]))
				i2++;
		}
	}

    "fast-import.c", line 858: warning: name redefined by pragma
redefine_extname declared static: tmpfile

ditto tempfile issue.

    "builtin/fast-export.c", line 54: warning: enum type mismatch: op "="

This seems to me to be a legitimate issue we introduced in 2d8ad46919213ebbd7bb72eb5b56cca8cc3ae07f, Elijah?

We're defining an enum like this:

    static enum { ABORT, VERBATIM, WARN, STRIP } signed_tag_mode = ABORT;
    static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ABORT;

And then doing:

	if (unset || !strcmp(arg, "abort"))
		tag_of_filtered_mode = ABORT; <-- Line 54
	else if (!strcmp(arg, "drop"))
		tag_of_filtered_mode = DROP;
	else if (!strcmp(arg, "rewrite"))
		tag_of_filtered_mode = REWRITE;

Presumably that assignment should be "= ERROR".

    "builtin/index-pack.c", line 175: warning: name redefined by
pragma redefine_extname declared static: tmpfile

ditto tempfile.

    "vcs-svn/string_pool.c", line 11: warning: initializer will be
sign-extended: -1
    "vcs-svn/string_pool.c", line 81: warning: initializer will be
sign-extended: -1
    "vcs-svn/repo_tree.c", line 112: warning: initializer will be
sign-extended: -1
    "vcs-svn/repo_tree.c", line 112: warning: initializer will be
sign-extended: -1
    "test-treap.c", line 34: warning: initializer will be sign-extended: -1

All of these come down to assigning ~0 to an uint32_t variable, e.g.:

	uint32_t token = ~0;

On Sun, Nov 6, 2011 at 5:33 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Looking back at that patch, "abort" was the word that made the most sense in the documentation and I was trying to make the code match. Unfortunately, that would result in "ABORT" being redefined (to the same value) so to squelch the compiler error I just went with the quick hack of changing "ABORT" in the enum definition to "ERROR", but didn't change "ABORT" later since "it had the same value anyway". Yeah, not the safest programming practice.

I probably should have just used FILTERED_TAG_ABORT and changed the signed_tag_mode enum to use SIGNED_TAG_ABORT (and then changed all references to "ABORT" to match one of those two)...or maybe somehow come up with a combined enum used for both (though WARN would only be useful fo signed_tag_mode, REWRITE would only be useful for tag_of_filtered_mode, etc.).