[0/3] remote API: fix -fanalyzer-spotted freeing issue

Message

Ævar Arnfjörð Bjarmason June 7, 2022, 3:50 p.m. UTC

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/[e-mail]/

Æ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 <[e-mail]>
     
      ## 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 <[e-mail]>
     
    @@ 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

Comments

Junio C Hamano June 7, 2022, 5:32 p.m. UTC | #1

Ævar Arnfjörð Bjarmason  <[e-mail]> writes:

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

[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.