RFC patch, 15 partsFix GCC -fanalyzer warnings & add -fanalyzer DEVOPTS mode

The -fanalyzer mode in GCC v12 is much better about reporting real issues. This RFC series currently conflicts with "next", but I thought sending it for comment sooner than later made sense.

With this series applied you can run:

    make DEVOPTS=analyzer

Which will turn all of these into warnings even under -Werror, we can also:

    make DEVOPTS="analyzer no-suppress-analyzer"

This works on both GCC 12 and GCC 11, but the former will yield better results. We need to quiet some issues on GCC 11. GCC 10 ships with "-fanalyzer", but it has so many false positives that we'll $(error) out if you try to run it with DEVOPTS=fanalyzer.

If you're CC'd on this series you were either involved in some discussion about -fanalyzer on the ML, or one of the commits mentioned in this series is yours. Comments on individual sets of patches below:

Ævar Arnfjörð Bjarmason (15):
  remote.c: don't dereference NULL in freeing loop
  pull.c: don't feed NULL to strcmp() on get_rebase_fork_point() path
  reftable: don't memset() a NULL from failed malloc()

These are all real bugs, in rough order of more to less severe.

  diff-lib.c: don't dereference NULL in oneway_diff()
  refs/packed-backend.c: add a BUG() if iter is NULL
  ref-filter.c: BUG() out on show_ref() with NULL refname

These may or may not be real bugs, I'm pretty sure they are...

  strbuf.c: placate -fanalyzer in strbuf_grow()

The first of cases where -fanalyzer isn't "wrong", but it is in the sense that the error it points out is a hard constraint that we (hopefully) ensure elsewhere in various APIs.

  strbuf.c: use st_add3(), not unsigned_add_overflows()

A while-we're-at-it for code spotted in the last commit.

  add-patch: assert parse_diff() expectations with BUG()
  reftable: don't have reader_get_block() confuse -fanalyzer
  blame.c: clarify the state of "final_commit" for -fanalyzer
  pack.h: wrap write_*file*() functions
  pack-write API: pass down "verify" not arbitrary flags

These are all cases where -fanalyzer flags "genuine" issues, but on closer inspection it's because we're passing the equivalent of two variables we know go hand-in-hand, but probably no compiler can be smart enough to spot that.

E.g. the first one here is a case where "git add -p" would segfault if fed diffs that aren't in the format "git diff" would emit, but since we control both sides it doesn't happen in practice.

Regardless, sprinkling assertions and BUG() guards in that code seems prudent.

  config.mak.dev: add a DEVOPTS=analyzer mode to use GCC's -fanalyzer

Add the new DEVOPTS mode and quiet some issues that need -Wno-error=*.

  config.mak.dev: add and use ASSERT_FOR_FANALYZER() macro

Quiet some outstanding issues, the main reason these aren't in commits like the above is because I didn't get to them yet, and may not any time soon. They may be genuine bugs, false alarms etc.

Even if we don't get to these I think having a CI mode with -fanalyzer and a whitelist of current issues would be an improment, since we'd error on *new* isssues.

This series doesn't add such a CI mode yet, due to conflicts with CI work in "next"..

Ævar Arnfjörð Bjarmason (15):
  remote.c: don't dereference NULL in freeing loop
  pull.c: don't feed NULL to strcmp() on get_rebase_fork_point() path
  reftable: don't memset() a NULL from failed malloc()
  diff-lib.c: don't dereference NULL in oneway_diff()
  refs/packed-backend.c: add a BUG() if iter is NULL
  ref-filter.c: BUG() out on show_ref() with NULL refname
  strbuf.c: placate -fanalyzer in strbuf_grow()
  strbuf.c: use st_add3(), not unsigned_add_overflows()
  add-patch: assert parse_diff() expectations with BUG()
  reftable: don't have reader_get_block() confuse -fanalyzer
  blame.c: clarify the state of "final_commit" for -fanalyzer
  pack.h: wrap write_*file*() functions
  pack-write API: pass down "verify" not arbitrary flags
  config.mak.dev: add a DEVOPTS=analyzer mode to use GCC's -fanalyzer
  config.mak.dev: add and use ASSERT_FOR_FANALYZER() macro

 Makefile                | 14 +++++++++
 add-patch.c             |  7 ++++-
 blame.c                 |  2 +-
 builtin/fast-import.c   |  2 +-
 builtin/index-pack.c    | 35 +++++++++-------------
 builtin/name-rev.c      |  1 +
 builtin/pack-objects.c  |  9 ++----
 builtin/pull.c          | 14 +++++----
 config.mak.dev          | 65 +++++++++++++++++++++++++++++++++++++++++
 diff-lib.c              |  3 ++
 dir.c                   |  1 +
 git-compat-util.h       | 16 ++++++++++
 gpg-interface.c         |  2 ++
 graph.c                 |  2 ++
 line-log.c              |  2 ++
 midx.c                  |  2 +-
 pack-write.c            | 38 +++++++++++-------------
 pack.h                  | 24 +++++++++------
 ref-filter.c            |  4 ++-
 refs/packed-backend.c   |  2 ++
 reftable/publicbasics.c |  2 ++
 reftable/reader.c       | 11 +++----
 remote.c                |  8 ++---
 strbuf.c                | 10 ++++---
 unpack-trees.c          |  1 +
 utf8.c                  |  1 +
 26 files changed, 195 insertions(+), 83 deletions(-)

Fix a bug in fd3cb0501e1 (remote: move static variables into per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i]) after having NULL'd out remote->pushurl. itself.

While we're at it let's get rid of the redundant braces per the CodingGuidelines, which also serves to show in the diff context that we were doing a FREE_AND_NULL(remote->pushurl) afterwards too, let's keep that one.

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

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:

The extended context is helping, but the brace removal makes this change harder to read.

So you remove the redundant release of the pushurl array, but the url array, which should be freed here after its elements are released, still leaks. The duplicate FREE_AND_NULL perhaps resulted from a copy&paste error.

Why set pushurl to NULL after release? This results in an invalid state unless pushurl_nr und pushurl_alloc are reset to zero. Same goes for the url array above -- either a simple free(3) call suffices or url_nr and url_alloc need to be cleared as well.

René Scharfe <l.s.r@web.de> writes:

We probably should give a huge warning next to FREE_AND_NULL() about this. It also is an effective way to hide an existing bug under the rug. diff_options.pathspec might be freed prematurely which may be noticed by a use-after-free if left to use free(), but FREE_AND_NULL() will mislead the use-after-free caller into thinkig that "ah there is no pathspec to be used" and produce nonsense result without crashing.

Thanks.

René Scharfe <l.s.r@web.de> writes:

This seems like the most likely explanation (even I don't recall how I ended up introducing this bug..). I probably meant to change pushurl -> url, but forgot somehow.

True, thanks for catching that. We'd probably never notice it because we never reuse the clear-ed `struct remote` (since we only ever call this when we clean up `struct repository`), but it's better to do the 'right thing'.

I didn't consider that when I chose to use FREE_AND_NULL() instead of free(), and there wasn't any particular reason why I chose one over the other.

Indeed, a small style fix in a larger change is one thing but here at least 80% of the changed lines are unrelated to the bugfix. I'm afraid my heart has started to sink when I see the phrase "while we're at it" in a commit message.

That's a good point

Best Wishes

Phillip

On Sat, Jun 04 2022, Phillip Wood wrote:

I'm happy to omit it, I thought it was helpful to force the context to be shown, will change that.

Fix an issue with feeding NULL to strcmp() noted by GCC's -fanalyzer, this fixes a bug in 1678b81ecce (pull: teach git pull about --rebase, 2015-06-18).

In cmd_pull() we could go through the function without initializing the "repo" argument (the -fanalyzer output shows how exactly), we'd then call get_rebase_fork_point (), which would in turn call get_tracking_branch() with that "NULL" repo argument.

Let's avoid this potential issue by returning NULL in this case, which will have get_rebase_fork_point() return -1 in turn.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pull.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:

This is equivalent to:

+	if (!*spec_src || !remote) {

It's easier to read without the redundant term.

The patch would be even easier to understand as a one-liner that just adds the missing check, i.e.:

- if (*spec_src) { + if (*spec_src && remote) {

Return NULL instead of possibly feeding a NULL to memset() in reftable_calloc(). This issue was noted by GCC 12's -fanalyzer:

	reftable/publicbasics.c: In function ‘reftable_calloc’:
	reftable/publicbasics.c:43:9: error: use of possibly-NULL ‘p’ where non-null expected [CWE-690] [-Werror=analyzer-possible-null-argument]
	   43 |         memset(p, 0, sz);
	      |         ^~~~~~~~~~~~~~~~
	[...]

This bug has been with us ever since this code was added in ef8a6c62687 (reftable: utility functions, 2021-10-07).

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

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:

We discussed this before, in https://lore.kernel.org/git/RFC-patch-2.2-364d1194a95-20220415T101740Z-avarab@gmail.com/

If this code was actually used by Git and still not handling allocation failures then I'd propose something like the below instead.

Next I'd probably try to convert reftable_calloc() calls to a variant that takes size and count separately -- like calloc(3) does -- to avoid unchecked multiplication.

--- >8 ---
Subject: [PATCH] reftable: remove reftable_set_alloc()

Use our native allocation functions for the reftable code to provide consistent handling of allocation failures. Remove the ability to change the allocator, as we don't need it anymore after this change.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 reftable/basics.h          |  7 -------
 reftable/publicbasics.c    | 43 --------------------------------------
 reftable/reftable-malloc.h | 18 ----------------
 reftable/system.h          |  5 +++++
 4 files changed, 5 insertions(+), 68 deletions(-)
 delete mode 100644 reftable/reftable-malloc.h

On Sat, Jun 04 2022, René Scharfe wrote:

I think this is a much better direction than my more narrow fix, and would be happy to see it queued up.

To your comment here & some others (e.g. FREE_AND_NULL()): I was really trying to focus on narrowly addressing these -fanalyzer issues without digressing into the larger topics "what is this code *really* doing, and does it make sense?". It was pretty unavoidable in 13/15 though.

Which isn't to say that I shouldn't fix some of it, e.g. your s/return/BUG()/ suggestion, but I think it's best to view these patches with an eye towards us already having these issues, and in most cases making -fanalyzer happy is a small cost.

And by doing so and getting a "clean build" we'll be able to turn it on in CI, and thus notice when we run into new -fanalyzer issues.

Am 04.06.22 um 02:54 schrieb Ævar Arnfjörð Bjarmason:

Future analyzer reports are likely of the same quality as the current ones. If the goal is to shush them then we should just not use the analyzer. If reports contain a helpful signal, e.g. pointing to a real bug or to overly complicated code, then we better address these issues.

We can think about automating the analyzer once we have a certain number of commits with improvements that would not have been made without it.

René

On Sat, Jun 04 2022, René Scharfe wrote:

We might decide not to go with -fanalyzer in CI or whatever, but I really think that your line of reasoning here is just the wrong way to evaluate the cost/benefit of -fanalyzer, a new warning or whatever.

There's ~15 commits in this series addressing things -fanalyzer brought up, and it would be ~20 if the remaining issues I punted on were addressed.

The question shouldn't be whether those things in particular were worth the effort, but whether the added safety of getting the new diagnostic going forward is worth the one-time cost.

Some of these commits are fixing issues going back to 2007-ish, $(git log --no-merges --oneline -- '*.[ch]' | wc -l) is ~25k lines. And looking at it like that 20/25K isn't that bad of a ratio :)

FWIW I spotted a couple of bugs in my own unsubmitted code from running all of it through -fanalyzer, and that POV is also worth thinking about, i.e. it's not just about improving git's current code, or even commits that might land in git.git in the future.

But also to provide a development aid so that when we're writing patches we spot issues earlier, even if they're ones we might spot before we send the patch, or in review before it gets applied.

It's also a much faster way of spotting certain issues, if you take into account that we've already been spotting some of these with the likes of SANITIZE=address, valgrind runs, or coverity.

I find the warning output from -fanalyzer to be *really useful*. It's scarily verbose at first, but it's basically doing most of the work for you in terms of exhaustively describing how the control flow got to a given location. With e.g. SANITIZE=address and valgrind (to the extent that they overlap) you might get a stacktrace or two, but you generally have to chase all that down yourself.

Am 04.06.22 um 18:23 schrieb Ævar Arnfjörð Bjarmason:

Is it pointing out real issues? Yes, the ones from patches 1 and 2 look legit. Let's get them fixed and attribute -fanalyzer for finding them! They are good selling points.

Is it giving us false positives? Patches 4, 5 and 6 look like that to me. Perhaps we can still use this opportunity to simplify the code for both human and machine, or at least add helpful asserts.

Patch 3 is a bit surprising to me. It's a real issue, but the whole reftable code never bothers to check for allocation errors, so I would have expected the analyzer to report lots more places.

Is it worth automating the thing? Perhaps, but we better work through the reports we already have first before getting new ones.

Sounds good, makes me want to try it. And I actually can, to a certain degree: GCC 13 seems to get confused when compiling compat/ code on my machine (macOS). Not sure what's going on. Compiling e.g. strbuf.c works, though, and gives me the same false positive. :-|

René

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

To me, your priorities look much less in line with this project's than what Rt Réne gave us above.

Workarounds for false positives are hardly one-time cost. They have to stay with us until the -fanalyzer gets corrected, somebody needs to remember to occasionally check if that happened, and revert the workaround to bring the code into their more natural form. What has been good and readable for human programmers for a long time does not need to be touched just to work around a false positive bug in a new tool.

I do not mind if you add -fanalyzer during your build via your own config.mak file, and you would help them improve the analyzer by reporting false positive bugs while finding possible bugs in the code we have (like you did in a few patches in this series) and the code you are writing. You are capable enough to keep your own set of patches to work around their false positive bugs locally.

But if you have to send in 15 patches with more workaround changes than real fix, then it is premature for us to bear the cost to have workaround for the tool.

There are folks who use our codebase as a suitably-sized guinea pig to improve their tool, and we should not make it harder for them to do so, but our priority is to improve the product of this project.

Come to think of it, adding unnecessary workarounds is a hostile act to those who are trying to improve -fanalyzer, I guess, too. They may want to fix problems in their tool, but workarounds hide them.

On Mon, Jun 06 2022, Junio C Hamano wrote:

Yes, but I think in this case most of these are either 100% legitimate issues, or things where we'd like to e.g. add a BUG() assertion anyway, e.g. in the diff parsing case.

Of course.

The idea here was to send an RFC showing what it would take to get it into a state where it would be more useful to others.

I.e. I've found it useful to run with it in my own builds and see if anything crops up that's not on the whitelist, I was aiming to give that to others with an easily tweakable knob.

Those people can still use CFLAGS=-fanalyzer

I'm not proposing that we do anything we wouldn't otherwise do to appease -fanalyzer, but that we:

 1. Fix things it alerts on because we'd find it worthwhile anyway
 2. For the rest, have a macro to appease it.

That macro being similar to e.g. UNLEAK() in that we'd opt-in enable it if you enabled certain flags, but otherwise we'd ignore it.

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

Where does that "BUG() assertion anyway" comes from? What are we protecting against?

I do not think such a BUG() added only for placating -fanalyzer falls into "legitimate" category at all.

On Mon, Jun 06 2022, Junio C Hamano wrote:

No, but e.g. in 09/15[1] we *would* have a segfault or NULL dereference if that function was fed input that it wasn't expecting, which is output from another git command (or the same output created in-core).

In that case we can reason about it being impossible in the current state, but annotating that with the equivalent of a BUG("unreachable") seems prudent, i.e. it helps human readers to understand the code, whatever -fanalyzer says.

Ditto the pack-related code in [2], where it's noting an aspect I'd noted in an earlier code review (before I'd ever used -fanalyzer). I.e. the combination of certain flags could land us in a NULL dereference, even if we can carefully reason that we won't combine the two having the code be less easily misunderstood seems better.

1. https://lore.kernel.org/git/RFC-patch-09.15-de0f7722608-20220603T183608Z-avarab@gmail.com/
2. https://lore.kernel.org/git/RFC-patch-13.15-63eeb66185a-20220603T183608Z-avarab@gmail.com/

Fix a control flow issue dating back to d1f2d7e8ca6 (Make run_diff_index() use unpack_trees(), not read_tree(), 2008-01-19) where we'd assume "tree" must be non-NULL if idx was NULL. As -fanalyzer shows we'd end up dereferencing "tree" in that case in ce_path_match():

dir.h:541:41: warning: dereference of NULL ‘ce’ [CWE-476] [-Wanalyzer-null-dereference]
  541 |                               S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode));
      |                                         ^
  ‘oneway_diff’: events 1-2
    |
    |diff-lib.c:493:12:
    |  493 | static int oneway_diff(const struct cache_entry * const *src,
    |      |            ^~~~~~~~~~~
    |      |            |
    |      |            (1) entry to ‘oneway_diff’
    |......
    |  506 |         if (tree == o->df_conflict_entry)
    |      |            ~
    |      |            |
    |      |            (2) following ‘true’ branch...
    |
  ‘oneway_diff’: event 3
    |
    |  507 |                 tree = NULL;
    |      |                      ^
    |      |                      |
    |      |                      (3) ...to here
    |
  ‘oneway_diff’: events 4-8
    |
    |  507 |                 tree = NULL;
    |      |                      ^
    |      |                      |
    |      |                      (4) ‘tree’ is NULL
    |  508 |
    |  509 |         if (ce_path_match(revs->diffopt.repo->index,
    |      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |             |
    |      |             (5) following ‘false’ branch (when ‘idx’ is NULL)...
    |      |             (6) ...to here
    |      |             (7) ‘tree’ is NULL
    |      |             (8) calling ‘ce_path_match’ from ‘oneway_diff’
    |  510 |                           idx ? idx : tree,
    |      |                           ~~~~~~~~~~~~~~~~~
    |  511 |                           &revs->prune_data, NULL)) {
    |      |                           ~~~~~~~~~~~~~~~~~~~~~~~~
    |
    +--> ‘ce_path_match’: event 9
           |
           |dir.h:535:19:
           |  535 | static inline int ce_path_match(struct index_state *istate,
           |      |                   ^~~~~~~~~~~~~
           |      |                   |
           |      |                   (9) entry to ‘ce_path_match’
           |
         ‘ce_path_match’: event 10
           |
           |  541 |                               S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode));
           |      |                                         ^
           |      |                                         |
           |      |                                         (10) dereference of NULL ‘ce

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 diff-lib.c | 3 +++
 1 file changed, 3 insertions(+)

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:

So here we have a D/F conflict in a oneway diff, i.e. a single tree. That means if we discard the thing from the tree, then we still have the conflicting thing from the index. Meaning idx and tree cannot both be NULL in that D/F conflict scenario. Right?

That calms down the confused compiler, but would it perhaps be better to BUG out at this point? Or is there a valid state with both idx and tree being NULL?

Adjust code added in 2775d8724d7 (packed_ref_store: implement reference transactions, 2017-09-08) to BUG() out in a case there GCC v12's -fanalyzer flagged that the "iter->oid" seen in the context was reachable where iter was NULL.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/packed-backend.c | 2 ++
 1 file changed, 2 insertions(+)

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:

Some peel backends dereference iter, e.g. packed_ref_iterator_peel() from the same file.

So if iter can be NULL at this point, then it must be checked before the ref_iterator_peel() call above, no?

*Can* it be NULL, though? The code in question is a bit complicated and it's late around here, but I cannot see how cmp < 0 and !iter can both be true at this place in the code. An optimizing compiler will probably optimize out the added check and string. Mine does (clang 13); you can check if yours does the same using:

  $ make refs/packed-backend.o && grep "must have" refs/packed-backend.o

Adjust code originally added in 5339bdad96a (ref-filter: introduce remote_ref_atom_parser(), 2016-02-17) to BUG() out rather than potentially segfault if we get a NULL refname here.

As noted by GCC v12's -fanalyzer that will happen if this follows the "refname = NULL" branch added in cc72385fe35 (for-each-ref: let upstream/push optionally report the remote name, 2017-10-05).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 ref-filter.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:

The related code not shown here looks tricky. IIUC refname cannot be NULL if option is RR_REF, because remote_ref_atom_parser() only sets .push_remote for other option values and only if that's set we can get a NULL refname passed to fill_remote_ref_details(). Right?

Do we need to have two different ways to indicate remote-ness? Having just one like after the patch below should reduce the risk of confusion.

Change the strbuf_grow() function so that GCC v12's -fanalyze doesn't yell at us about sb->buf[0] dereferencing NULL, this also makes this code easier to follow.

This BUG() should be unreachable since the state of our "sb->buf" and "sb->alloc" goes hand-in-hand, but -fanalyzer isn't smart enough to know that, and adding the BUG() also makes it clearer to human readers that that's what happens here.

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

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:

new_buf is !sb->alloc. ALLOC_GROW sets buf if sb->len + extra + 1 is bigger than sb->alloc, which is always true because the variables in the sum are unsigned, so we end up with at least 1 > 0. That's easy enough to see; I wonder why the compiler doesn't agree. Am I missing something?

Does setting the attribute returns_nonnull for xrealloc help? Specifying it explicitly makes sense to me -- expecting the compiler to infer it automatically across compilation units is probably a bit too much to ask for, or at least needlessly expensive.

Hi Ævar

[This is an old address that I only have webmail access to, please use phillip.wood@dunelm.org.uk when cc'ing me]

This is a bit ugly, have you tried adding __attribute__((malloc (free), returns_nonnull)) to xmalloc() and xrealloc() ?

Best Wishes

Phillip

On Sat, Jun 04 2022, Phillip Wood wrote:

Hrm, I just grabbed it from an old commit of yours I found. Consider sending in a patch to update .mailmap :)

Will try to experiment with that, perhaps GCC can be massaged to grok this somehow.

I do vaguely remember (but couldn't track down where it was) that we have some config for coverity for this function, due to it also having trouble with it.

Am 04.06.22 um 18:21 schrieb Ævar Arnfjörð Bjarmason:

Good idea, but this attribute doesn't seem to help here.

The following helps, but I don't know why it would be needed -- if alloc is 0 then any strbuf_grow() call will give a nr of at least 1 (for the NUL character):

On 04/06/2022 21:37, René Scharfe wrote:

Indeed, I was tricked by the misleading BUG message, the analyzer is not complaining that xrealloc() may return NULL, it is taking a path where it does not reallocate the buffer. I've copied the full output at the end of the message if anyone wants to see the whole thing but the relevant part is

            |cache.h:727:20:
            |  727 |                 if ((nr) > alloc) { \
            |      |                    ^
            |      |                    |
            |      |                    (10) following ‘false’ branch...
strbuf.c:99:9: note: in expansion of macro ‘ALLOC_GROW’
            |   99 |         ALLOC_GROW(sb->buf, sb->len + extra + 1, 
sb->alloc);
            |      |         ^~~~~~~~~~

Yes it seems to be ignoring the result of our overflow checks and assuming that sb->len + extra + 1 can overflow to zero. Having seen the number of false positives I'm inclined to think we should take the bug fixes and punt the rest of these patches. Maybe after a couple more gcc releases the false positive rate will be more manageable. (I tired running gcc 11 with -fanalyzer a few months ago and gave up looking at the output before I found a single real bug so it has certainly improved in gcc 12)

Best Wishes

Phillip

strbuf.c: In function ‘strbuf_grow’:
strbuf.c:101:28: warning: dereference of NULL ‘0’ [CWE-476] 
[-Wanalyzer-null-dereference]
   101 |                 sb->buf[0] = '\0';
       |                 ~~~~~~~~~~~^~~~~~
   ‘strbuf_normalize_path’: events 1-2
     |
     | 1156 | int strbuf_normalize_path(struct strbuf *src)
     |      |     ^~~~~~~~~~~~~~~~~~~~~
     |      |     |
     |      |     (1) entry to ‘strbuf_normalize_path’
     |......
     | 1160 |         strbuf_grow(&dst, src->len);
     |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~
     |      |         |
     |      |         (2) calling ‘strbuf_grow’ from ‘strbuf_normalize_path’
     |
     +--> ‘strbuf_grow’: events 3-4
            |
            |   91 | void strbuf_grow(struct strbuf *sb, size_t extra)
            |      |      ^~~~~~~~~~~
            |      |      |
            |      |      (3) entry to ‘strbuf_grow’
            |......
            |   94 |         if (unsigned_add_overflows(extra, 1) ||
            |      |            ~
            |      |            |
            |      |            (4) following ‘false’ branch (when 
‘extra != 18446744073709551615’)...
            |
          ‘strbuf_grow’: event 5
            |
            |   95 |             unsigned_add_overflows(sb->len, extra + 1))
git-compat-util.h:128:7: note: in definition of macro 
‘unsigned_add_overflows’
            |  128 |     ((b) > maximum_unsigned_value_of_type(a) - (a))
            |      |       ^
            |
          ‘strbuf_grow’: events 6-9
            |
            |strbuf.c:94:46:
            |   94 |         if (unsigned_add_overflows(extra, 1) ||
            |......
            |   97 |         if (new_buf)
            |      |         ~~ ~
            |      |         |  |
            |      |         |  (8) following ‘true’ branch...
            |      |         (7) ...to here
            |   98 |                 sb->buf = NULL;
            |      |                 ~~
            |      |                 |
            |      |                 (9) ...to here
            |
          ‘strbuf_grow’: event 10
            |
            |cache.h:727:20:
            |  727 |                 if ((nr) > alloc) { \
            |      |                    ^
            |      |                    |
            |      |                    (10) following ‘false’ branch...
strbuf.c:99:9: note: in expansion of macro ‘ALLOC_GROW’
            |   99 |         ALLOC_GROW(sb->buf, sb->len + extra + 1, 
sb->alloc);
            |      |         ^~~~~~~~~~
            |
          ‘strbuf_grow’: event 11
            |
            |cache.h:726:12:
            |  726 |         do { \
            |      |            ^
            |      |            |
            |      |            (11) ...to here
strbuf.c:99:9: note: in expansion of macro ‘ALLOC_GROW’
            |   99 |         ALLOC_GROW(sb->buf, sb->len + extra + 1, 
sb->alloc);
            |      |         ^~~~~~~~~~
            |
          ‘strbuf_grow’: events 12-15
            |
            |  100 |         if (new_buf)
            |      |            ^
            |      |            |
            |      |            (12) following ‘true’ branch...
            |  101 |                 sb->buf[0] = '\0';
            |      |                 ~~~~~~~~~~~~~~~~~
            |      |                 | |        |
            |      |                 | |        (15) dereference of NULL 
‘*sb.buf’
            |      |                 | (14) ‘dst.buf’ is NULL
            |      |                 (13) ...to here

Change the strbuf_grow() function to use st_add3() instead of doing its own unsigned_add_overflows() checks. The overflow checking here was originally added in b449f4cfc97 (Rework strbuf API and semantics., 2007-09-06) and adjusted in 1368f65002b (compat: helper for detecting unsigned overflow, 2010-10-10). Instead we compute a "sz" with st_add3().

That was done at a time when the underlying xrealloc() in git-compat-util.h didn't use st_mult() yet, that has been the case since the later e7792a74bcf (harden REALLOC_ARRAY and xcalloc against size_t overflow, 2016-02-22).

The only behavior change here should be the very obscure edge case that we'd previously die() in cases where we strictly didn't need to, as we'd check both "extra + 1" and "sb->len + extra + 1" for overflow. If we overflowed only on the latter but were doing the former we'd needlessly die() die. I don't think that difference mattered, but it's noted here for completeness.

While we're at it add an inline comment about why we're adding 1 to the values, that's also explained in the API documentation in strbuf.h, but since we're using that magic constant here...

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

Am 03.06.22 um 20:37 schrieb Ævar Arnfjörð Bjarmason:

Good idea.

How is that relevant?

st_add3 does the same checks.  What do you mean with "doing the former"?

This is a change in behavior. The one you meant above? It ignores the len value if alloc is zero. Any len value other than zero is invalid in that case. Adding code to paper over this invalid state seems a waste. And if a brave caller fiddled with len before calling strbuf_grow(), the resulting allocation will now be shorter. Hopefully nobody does that, but it's not totally impossible.

We could report the issue instead with something like the following:

	if (new_buf && sb->len)
		BUG("unallocated strbuf has invalid len: %"PRIuMAX, (uintmax_t)sb->len);

I'd rather keep the old behavior. But whatever we decide to do, I think the st_add3 change deserves its own commit with no further change in behavior.

I don't mind the added comment.

Assert that this code added in [1], [2] and other related commits expects that once we see a "diff " line we should have a non-NULL "file_diff" and "hunk".

In practice this would have always been the case, as we are parsing our own "diff" output, but e.g. GCC v12's -fanalyzer doesn't know that, and will alert us that in the "else if" and below in this function we could be dereferencing NULL if we were processing anything except our expected input.

1. f6aa7ecc343 (built-in add -i: start implementing the `patch`
   functionality in C, 2019-12-13)
2. 80399aec5ab (built-in add -p: support multi-file diffs, 2019-12-13)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 add-patch.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Hi Ævar

If we're only doing this to keep -fanalyzer quiet then would it be better to use the macro you introduce at the end of this series instead?

Best Wishes

Phillip

Change the control flow in reftable code add in 46bc0e731a7 (reftable: read reftable files, 2021-10-07) to work around a false positive spotted by GCC's -fanalyzer option[1]. The code was added in 46bc0e731a7 (reftable: read reftable files, 2021-10-07).

Usually we'd just leave such false positives alone, but having looked at it the control flow was also odd and confusing to humans, so let's change it.

What -fanalyzer complained about was:

	|......
	|  294 |         if (next_off >= r_size)
	|      |            ~
	|      |            |
	|      |            (24) following ‘false’ branch (when ‘next_off < r_size’)...
	|......
	|  294 |         if (next_off >= r_size)
	|      |            ~
	|      |            |
	|      |            (24) following ‘false’ branch (when ‘next_off < r_size’)...
	|......
	|  297 |         err = reader_get_block(r, &block, next_off, guess_block_size, r->size);
	|      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
	|      |               |
	|      |               (25) ...to here
	|      |               (26) calling ‘reader_get_block’ from ‘reader_init_block_reader’
	|
	+--> ‘reader_get_block’: events 27-29
	       |
	       |   59 | static int reader_get_block(struct reftable_reader *r,
	       |      |            ^~~~~~~~~~~~~~~~
	       |      |            |
	       |      |            (27) entry to ‘reader_get_block’
	       |......
	       |   63 |         if (off >= r_size)
	       |      |            ~
	       |      |            |
	       |      |            (28) following ‘true’ branch (when ‘off >= r_size’)...
	       |   64 |                 return 0;
	       |      |                        ~
	       |      |                        |
	       |      |                        (29) ...to here
	       [...]
	       |  275 |         *typ = data[0];
	       |      |                ~~~~~~~
	       |      |                    |
	       |      |                    (37) ...to here
	       |      |                    (38) dereference of NULL ‘data’

I.e. it thought that we could take the "return 0" in reader_get_block() due to "(off >= r->size)", which followed the identical "(next_off >= r_size)" check in reader_init_block_reader() just before reader_get_block() was called.

But whatever GCC's -fanalyzer thinks it's confusing that we're checking this twice, and making it look as though these parameters might change within the reader_init_block_reader() function, but they won't. So let's just do the check once early, and use "const" types to assert that they'll be constant throughout.

1. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105285

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 reftable/reader.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Change code added in d0d0ef1f67c (blame: create scoreboard setup function, 2017-05-24) so that GCC v12's -fanalyzer doesn't think that we can have a "NULL" final_commit in the modified branch.

This happens because the analyzer gives up in the prepare_revision_walk() function, and thinks that the "sb->reverse && sb->revs->first_parent_only" condition we already checked a few lines above can have a different result at this point.

That isn't the case, but what we really mean here is "if we previously set up the final commit [because that was true]", so let's do that instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 blame.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Add an option to compile with GCC's -fanalyzer, which as noted in [1] and [2] has become much more useful in the recently released GCC v12 series.

Here we're quieting a few outstanding -fanalyzer issues that require us to use -Wno-error=* on an entire file:

 * range-diff.c, because it involves loop variables and would be
   painfully verbose to instrument with the ASSERT_FOR_FANALYZER() macro
   introduced in the subsequent commit.

 * http-fetch.c and fsmonitor-settings.c, because those aren't issues
   where we're referencing NULL, and therefore we can't quiet it with an
   assert().

For non-GCC compilers I considered wrapping the DEVOPTS logic in:

	ifneq ($(filter gcc10,$(COMPILER_FEATURES)),)
	endif

Which would make it OK to specify DEVOPTS=analyzer under other compilers, or on older GCC. But then we'd silently ignore the option on those. Let's instead trust the compiler to error out if it doesn't support -fanalyzer.

There are various limitations and bugs in the analyzer engine, e.g. I filed [3] for a false positive in builtin/merge-file.c before GCC v12 was released, which was subsequently fixed in GCC 12 trunk in [4], but many other issues remain.

1. https://developers.redhat.com/articles/2022/04/12/state-static-analysis-gcc-12-compiler
2. https://gcc.gnu.org/gcc-12/changes.html
3. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105264
4. https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=a358e4b60815b41e27f3508014ceb592f86b9b45

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile       | 14 ++++++++++++
 config.mak.dev | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

Whitespace-wrap some overly long write_*file*() lines in preparation for changing their signatures in a subsequent commit. I named the "opts" argument to "write_idx_file()" while I was at it for consistency.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 pack.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Change the pack-write API to accept a boolean "verify" parameter instead of passing down the "struct pack_idx_option" flags directly.

This simplifies the code for both humans and machines, e.g. GCC's -fanalyzer would correctly note that there were potential paths through this code where we'd deference NULL, but in reality we wouldn't hit them because certain flags would always go hand-in-hand.

Let's instead separate the underlying API from the total set of flags, and stop passing the flags down to functions that don't need the full set of flags, they'll just get the specific state they need.

See e37d0b8730b (builtin/index-pack.c: write reverse indexes, 2021-01-25) for the initial implementation, and [1] for the initial WIP version of this commit on the ML.

1. https://lore.kernel.org/git/87v93bidhn.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fast-import.c  |  2 +-
 builtin/index-pack.c   | 35 ++++++++++++++---------------------
 builtin/pack-objects.c |  9 +++------
 midx.c                 |  2 +-
 pack-write.c           | 38 +++++++++++++++++---------------------
 pack.h                 | 15 ++++++---------
 6 files changed, 42 insertions(+), 59 deletions(-)

Add an ASSERT_FOR_FANALYZER() macro to quiet those -fanalyzer issues that haven't been diagnosed yet to either change the code involved, or to add an assert() or BUG() to it to placate GCC v12's -fanalyzer mode.

As in the preceding commit we could also use -Wno-error=* here, which was the initial implementation in config.mak.dev, i.e. something like:

	## -Wno-error=analyzer-null-dereference
	$(eval $(call fn_disable_analyzer, \
		-Wno-error=analyzer-null-dereference, \
		dir \
		gpg-interface \
		graph \
		range-diff \
		utf8 \
	))
	## -Wno-error=analyzer-null-dereference: pre-gcc12
	ifeq ($(filter gcc12,$(COMPILER_FEATURES)),)
	$(eval $(call fn_disable_analyzer, \
		-Wno-error=analyzer-null-dereference, \
		merge \
		builtin/name-rev \
	))
	endif

But any such approach will unfortunately quiet *all* in the relevant files, including any new ones. Instead we want to quiet specific issues involved with ASSERT_FOR_FANALYZER().

A drawback of this overall approach is that this only works under e.g. CFLAGS=-O0, but not CFLAGS=-O3. The compiler takes the liberty to re-arrange our code to get around these assert()s, and other new issues will also crop up. All of this is expected (-fanalyzer is subject to optimization passes, like most other options), but let's focus on -O0 for now.

Commentary on specific cases:

 * builtin/name-rev.c: Since 45a14f578e1 (Revert "name-rev: release
   unused name strings", 2022-04-22) GCC v12's -fanalyzer has complained
   about this code, per René's analysis in [1] it's incorrect to do
   so. So let's add an ASSERT_FOR_FANALYZER() to placate it.

 * graph.c: In 0f0f389f120 (graph: tidy up display of left-skewed
   merges, 2019-10-15) a previous "assert" was removed, and another
   unconditional deference of the return value of
   first_interesting_parent() was added without checking it. Since it
   can return NULL let's add ASSERT_FOR_FANALYZER()'s here to note for
   -fanalyzer that we won't be dereferencing these in practice.

1. https://lore.kernel.org/git/dece627d-ccf8-2375-0c50-c59637e561d3@web.de/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/name-rev.c |  1 +
 config.mak.dev     |  4 ++++
 dir.c              |  1 +
 git-compat-util.h  | 16 ++++++++++++++++
 gpg-interface.c    |  2 ++
 graph.c            |  2 ++
 line-log.c         |  2 ++
 unpack-trees.c     |  1 +
 utf8.c             |  1 +
 9 files changed, 30 insertions(+)

If the purpose of the macro is to silence -fanalyzer then it would perhaps be better to name it after that purpose rather than the way that it is implemented. SILENCE_FANALYZER_WARNING() or something like that.

I had a hard time understanding this, maybe

But any such approach will unfortunately miss any new warnings as it silences them for the whole file.

Best Wishes

Phillip

This series is spun off from the recent RFC -fanalyzer series[1] and fixes a clear bug in remote_state_clear().

While doing so remove the underlying source of the landmines in the API and use free() rather than FREE_AND_NULL() for structs that aren't being re-used.

Then have the API free() the structure itself, rather than have *_new() allocate it, but the caller being responsible for calling both remote_state_clear() and free().

1. https://lore.kernel.org/git/RFC-cover-00.15-00000000000-20220603T183608Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (3):
  remote.c: remove braces from one-statement "for"-loops
  remote.c: don't dereference NULL in freeing loop
  remote API: don't buggily FREE_AND_NULL(), free() instead

 remote.c     | 23 ++++++++++-------------
 remote.h     | 10 +++++++++-
 repository.c |  2 +-
 3 files changed, 20 insertions(+), 15 deletions(-)

Range-diff:
 1:  b3a678d934a !  1:  1879ed2826e remote.c: don't dereference NULL in freeing loop
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    remote.c: don't dereference NULL in freeing loop
    +    remote.c: remove braces from one-statement "for"-loops
     
    -    Fix a bug in fd3cb0501e1 (remote: move static variables into
    -    per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i])
    -    after having NULL'd out remote->pushurl. itself.
    -
    -    While we're at it let's get rid of the redundant braces per the
    -    CodingGuidelines, which also serves to show in the diff context that
    -    we were doing a FREE_AND_NULL(remote->pushurl) afterwards too, let's
    -    keep that one.
    +    Remove braces that don't follow the CodingGuidelines from code added
    +    in fd3cb0501e1 (remote: move static variables into per-repository
    +    struct, 2021-11-17). A subsequent commit will edit code adjacent to
    +    this.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ remote.c: static void remote_clear(struct remote *remote)
     +	for (i = 0; i < remote->url_nr; i++)
      		free((char *)remote->url[i]);
     -	}
    --	FREE_AND_NULL(remote->pushurl);
    --
    + 	FREE_AND_NULL(remote->pushurl);
    + 
     -	for (i = 0; i < remote->pushurl_nr; i++) {
     +	for (i = 0; i < remote->pushurl_nr; i++)
      		free((char *)remote->pushurl[i]);
    @@ remote.c: static void remote_clear(struct remote *remote)
      	FREE_AND_NULL(remote->pushurl);
      	free((char *)remote->receivepack);
      	free((char *)remote->uploadpack);
    +@@ remote.c: void remote_state_clear(struct remote_state *remote_state)
    + {
    + 	int i;
    + 
    +-	for (i = 0; i < remote_state->remotes_nr; i++) {
    ++	for (i = 0; i < remote_state->remotes_nr; i++)
    + 		remote_clear(remote_state->remotes[i]);
    +-	}
    + 	FREE_AND_NULL(remote_state->remotes);
    + 	remote_state->remotes_alloc = 0;
    + 	remote_state->remotes_nr = 0;
 2:  4a055969ea5 <  -:  ----------- pull.c: don't feed NULL to strcmp() on get_rebase_fork_point() path
 3:  0b570d112fc <  -:  ----------- reftable: don't memset() a NULL from failed malloc()
 4:  3a287c19d7e <  -:  ----------- diff-lib.c: don't dereference NULL in oneway_diff()
 5:  46e0c307941 <  -:  ----------- refs/packed-backend.c: add a BUG() if iter is NULL
 6:  2d04035d7aa <  -:  ----------- ref-filter.c: BUG() out on show_ref() with NULL refname
 7:  cf1a5f3ed0f <  -:  ----------- strbuf.c: placate -fanalyzer in strbuf_grow()
 8:  2c4b7832144 <  -:  ----------- strbuf.c: use st_add3(), not unsigned_add_overflows()
 9:  de0f7722608 <  -:  ----------- add-patch: assert parse_diff() expectations with BUG()
10:  b50558d3b24 <  -:  ----------- reftable: don't have reader_get_block() confuse -fanalyzer
11:  66518467e1d <  -:  ----------- blame.c: clarify the state of "final_commit" for -fanalyzer
12:  9f0f515ed3a <  -:  ----------- pack.h: wrap write_*file*() functions
13:  63eeb66185a <  -:  ----------- pack-write API: pass down "verify" not arbitrary flags
14:  9cf550688d4 <  -:  ----------- config.mak.dev: add a DEVOPTS=analyzer mode to use GCC's -fanalyzer
15:  16bd2270b4c <  -:  ----------- config.mak.dev: add and use ASSERT_FOR_FANALYZER() macro
 -:  ----------- >  2:  0e258c230f6 remote.c: don't dereference NULL in freeing loop
 -:  ----------- >  3:  062fb3f454e remote API: don't buggily FREE_AND_NULL(), free() instead

Remove braces that don't follow the CodingGuidelines from code added in fd3cb0501e1 (remote: move static variables into per-repository struct, 2021-11-17). A subsequent commit will edit code adjacent to this.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 remote.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Fix a bug in fd3cb0501e1 (remote: move static variables into per-repository struct, 2021-11-17) where we'd free(remote->pushurl[i]) after having NULL'd out remote->pushurl. itself. We free "remote->pushurl" in the next "for"-loop, so doing this appears to have been a copy/paste error.

Before this change GCC 12's -fanalyzer would correctly note that we'd dereference NULL in this case, this change fixes that:

	remote.c: In function ‘remote_clear’:
	remote.c:153:17: error: dereference of NULL ‘*remote.pushurl’ [CWE-476] [-Werror=analyzer-null-dereference]
	  153 |                 free((char *)remote->pushurl[i]);
	      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
	      [...]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

Wow, that's a bad one. Why didn't anybody notice this at runtime, I have to wonder.

Thanks.

Change the buggy "remote_clear()" function to stop pretending to to be able to zero out a "struct remote". Setting "url" and "pushurl" to NULL results in an invalid state unless the corresponding "url_nr" and "pushurl_nr" are also set to zero.

In this case however we do not use the "struct remote", so the FREE_AND_NULL() pattern added in fd3cb0501e1 (remote: move static variables into per-repository struct, 2021-11-17) can be replaced with free().

The API was also odd in that remote_state_new() would xmalloc() for us, but the user had to free() it themselves, let's instead change the behavior to have the destructor free() what we malloc() in the constructer.

In this case this appears to have been done for consistency with repo_clear(), let's instead have repo_clear() handle the NULL-ing of its "remote_state", and not attempt to reset the structure in remote.c

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 remote.c     | 14 +++++++-------
 remote.h     | 10 +++++++++-
 repository.c |  2 +-
 3 files changed, 17 insertions(+), 9 deletions(-)

Thanks for sending out this series :) I feel a little guilty leaving behind this much junk. Coincidentally, I was already planning on revisiting my previous work to clean up debt/mistakes, so this was a good lesson in what to look out for.

The previous patches look good to me, and I have no comments on those. I have a style question on this patch (more for my own learning than a suggestion on this patch), but it also looks good to me.

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

Did you mean *reuse* the "struct remote"?

Yes, this was specifically for consistency reasons. We'll have to read on to see whether or not this patch remains consistent with repo_clear()...

Now that remote_state_clear() free()-s the "struct remote_state", should we rename it to something like "remote_state_free()"? There's some precedent for this in the other repo_clear() 'destructors'...

I suppose the question of whether or not to free() in the 'destructor' depends on whether we expect the struct to be reusable? I don't expect that "struct remote_state" needs to be reused, so free()-ing it is ok to me.

The API is not _that_ odd though ;) As you noted, my initial use of FREE_AND_NULL() is for consistency reasons with the rest of repo_clear(), which looks like this:

	if (repo->config) {
		git_configset_clear(repo->config);
		FREE_AND_NULL(repo->config);
	}

	if (repo->submodule_cache) {
		submodule_cache_free(repo->submodule_cache);
		repo->submodule_cache = NULL;
	}

	if (repo->index) {
		discard_index(repo->index);
		if (repo->index != &the_index)
			FREE_AND_NULL(repo->index);
	}

	if (repo->promisor_remote_config) {
		promisor_remote_clear(repo->promisor_remote_config);
		FREE_AND_NULL(repo->promisor_remote_config);
	}

	if (repo->remote_state) {
		remote_state_clear(repo->remote_state);
 -	FREE_AND_NULL(repo->remote_state);
 +	repo->remote_state = NULL;
	}

promisor_remote_clear(), discard_index(), and git_configset_clear() don't free() the struct, so it makes sense for them to use FREE_AND_NULL(). AFAICT, these structs are meant to be reused, so it makes sense that we "clear" it without freeing the struct pointer itself.

On the other hand, submodule_cache_free() _does_ free() the struct, and so we just use "= NULL". I noticed that this uses the verb "free", and not "clear".

So now that remote_state_clear() *does* free() the struct, it is perfectly fine to use "= NULL" here as well, though it uses the verb "clear".

I'm not sure if we have a style around clear/free. Feel free to ignore if there isn't one.

Glen Choo <chooglen@google.com> writes:

So git_configset_clear() does clear but does not free.

submodule_cache_free() does (probably clear and) free.

discard_index() does not free.

promisor_remote_clear() does not free.

It does bother me. Changing _clear() that did not free the container resource to free it, without changing the name to free or release or whatever, smells like leaving a source of confusion for future developers.

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

"buggy" -> ""; "to to" -> "to".

Over-using FREE_AND_NULL() is indeed irritating, but as long as we are not reusing the struct whose member are getting freed, there is no "bug" per-se, no?

"use" -> "reuse", with comma around "however" on both sides, probably.

Yay.

I am not sure if remote_clear() that does not free the remote goes well with remote_state_clear() that does free remote_state. As long as it is clear whose responsibility to release resources is, we can go either way, but helper functions with similar names having quite different resource maintainance semantics looks like a potential source of confusion to me.

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

[1/3] is a pretty-much "Meh" (read: if it is already in the code, it is not worth a patch noise to go and fix it, even though we would avoid newly introducing the construct in new code), and some part of [3/3] I am not so sure about (read: it is a mixed bag of changes), but [2/3] is a bug that I find surprising that nobody noticed at runtime.

Thanks.  Will queue.