Fwd: New Defects reported by Coverity Scan for git

Fwd: New Defects reported by Coverity Scan for git

[Duy Nguyen]

I think Coverity caught this correctly.

** CID 1306846:  Memory - illegal accesses  (USE_AFTER_FREE)
/builtin/pull.c: 287 in config_get_rebase()


________________________________________________________________________________________________________
*** CID 1306846:  Memory - illegal accesses  (USE_AFTER_FREE)
/builtin/pull.c: 287 in config_get_rebase()
281
282             if (curr_branch) {
283                     char *key = xstrfmt("branch.%s.rebase",
curr_branch->name);
284
285                     if (!git_config_get_value(key, &value)) {
286                             free(key);
>>>     CID 1306846:  Memory - illegal accesses  (USE_AFTER_FREE)
>>>     Passing freed pointer "key" as an argument to "parse_config_rebase".
287                             return parse_config_rebase(key, value, 1);
288                     }
289
290                     free(key);
291             }
292
-- 
Duy

Fwd: New Defects reported by Coverity Scan for git

[Duy Nguyen]

Jeff, I suppose you are the admin of git on scan.coverity, or knows
him/her, perhaps we can add a model for xmalloc to suppress these
"null pointer deferences" reports? We are sure xmalloc() never returns
NULL. Qemu did it [1] and it looks simple.. I think something like
this would do

void *xmalloc(size_t size)
{
   void *mem = malloc(size);
   if (!mem) __coverity_panic__();
   return mem;
}

[1] http://git.qemu.org/?p=qemu.git;a=blob;f=scripts/coverity-model.c;h=4c99a85cfc292caa9edd9d041e2683ee53490a8d;hb=e40cdb0e6efb795e4d19368987d53e3e4ae19cf7#l104


---------- Forwarded message ----------
From:  <scan-admin@coverity.com>
Date: Fri, Jul 31, 2015 at 5:54 PM
Subject: New Defects reported by Coverity Scan for git
To: pclouds@gmail.com

_______________________________________________________________________________________________________
*** CID 1313836:  Null pointer dereferences  (FORWARD_NULL)
/rerere.c: 150 in find_rerere_dir()
144                     return NULL; /* BUG */
145             pos = sha1_pos(sha1, rerere_dir, rerere_dir_nr,
rerere_dir_sha1);
146             if (pos < 0) {
147                     rr_dir = xmalloc(sizeof(*rr_dir));
148                     hashcpy(rr_dir->sha1, sha1);
149                     rr_dir->status_nr = rr_dir->status_alloc = 0;
>>>     CID 1313836:  Null pointer dereferences  (FORWARD_NULL)
>>>     Assigning: "rr_dir->status" = "NULL".
150                     rr_dir->status = NULL;
151                     pos = -1 - pos;
152
153                     /* Make sure the array is big enough ... */
154                     ALLOC_GROW(rerere_dir, rerere_dir_nr + 1,
rerere_dir_alloc);
155                     /* ... and add it in. */

** CID 1313835:  Null pointer dereferences  (FORWARD_NULL)
/builtin/fetch.c: 795 in prune_refs()
-- 
Duy

Fwd: New Defects reported by Coverity Scan for git

[Stefan Beller]

Not sure what triggered the new finding of coverity as seen below as the
parse_commit() was not touched. Junios series regarding the merge base
optimization touches a bit of code nearby though.

Do we want to replace the unchecked places of parse_commit with
parse_commit_or_die ?

Thanks,
Stefan
_________________________________________________________
*** CID 1374088:  Error handling issues  (CHECKED_RETURN)
/commit.c: 913 in mark_redundant()
907
908             work = xcalloc(cnt, sizeof(*work));
909             redundant = xcalloc(cnt, 1);
910             ALLOC_ARRAY(filled_index, cnt - 1);
911
912             for (i = 0; i < cnt; i++)
>>>     CID 1374088:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "parse_commit" without checking return value (as is done elsewhere 37 out of 45 times).
913                     parse_commit(array[i]);
914             for (i = 0; i < cnt; i++) {
915                     struct commit_list *common;
916
917                     if (redundant[i])
918                             continue;

Re: Fwd: New Defects reported by Coverity Scan for git

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Not sure what triggered the new finding of coverity as seen below as the
> parse_commit() was not touched. Junios series regarding the merge base
> optimization touches a bit of code nearby though.
>
> Do we want to replace the unchecked places of parse_commit with
> parse_commit_or_die ?

The reason parse_commit() would fail at this point would be because
the repository is corrupt, I do not think it would hurt to do such a
change.  

I agree that it is curious why it shows up as a "new defect",
though.

By the way, do you know who is managing the service on our end
(e.g. approving new people to be "defect viewer")?  The site seems
to think I have the power to manage others' subscription, which I do
not think I have (I do not go to the site myself).  As it spewed
quite a many false positives into my mailbox in the past, I do not
pay very close attention to these reports these days, but I still
read the e-mailed reports every once in a while.

Thanks.

Re: Fwd: New Defects reported by Coverity Scan for git

[Stefan Beller]

On Thu, Oct 20, 2016 at 10:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Not sure what triggered the new finding of coverity as seen below as the
>> parse_commit() was not touched. Junios series regarding the merge base
>> optimization touches a bit of code nearby though.
>>
>> Do we want to replace the unchecked places of parse_commit with
>> parse_commit_or_die ?
>
> The reason parse_commit() would fail at this point would be because
> the repository is corrupt, I do not think it would hurt to do such a
> change.
>
> I agree that it is curious why it shows up as a "new defect",
> though.
>
> By the way, do you know who is managing the service on our end
> (e.g. approving new people to be "defect viewer")?

I do it most of the time, but I did not start managing it.
And I have been pretty lax/liberal about handing out rights to do stuff,
because I did not want to tip on anyone's toes giving to few rights
and thereby annoying them.

I see that some of these emails may be inconvenient to you, I can
change your role to defect viewer/contributor if you prefer.

Thanks,
Stefan

Re: Fwd: New Defects reported by Coverity Scan for git

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> I do it most of the time, but I did not start managing it.
> And I have been pretty lax/liberal about handing out rights to do stuff,
> because I did not want to tip on anyone's toes giving to few rights
> and thereby annoying them.

Good to know that you have been managing it; I was mostly worried
about not having anybody managing it (i.e. imagining Coverity
nominated/volunteered me as manager with everybody else as viewers)
and the new viewer requests get totally ignored by the project as
the whole.

> I see that some of these emails may be inconvenient to you, I can
> change your role to defect viewer/contributor if you prefer.

It is not a huge inconvenience to me, because any piece of e-mail
that is addressed directly to gitster@ without CC'ing to git@vger
and is not a follow-up to any earlier message goes to a separate
lowest-priority mailbox that I rarely look at.  But if it is easy
to recategorize me, please do so.

Thanks.

[PATCH] commit parsing: replace unchecked parse_commit by parse_commit_or_die

[Stefan Beller]

The reason parse_commit() would fail at these points would be because
the repository is corrupt.

This was noticed by coverity.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

 developed on pu as that's where coverity spotted it.
 I have no overview if these areas are being worked on. (It may clash
 with at least jc/merge-base-fp-only)
 
 Thanks,
 Stefan

 builtin/blame.c       | 2 +-
 builtin/describe.c    | 4 ++--
 builtin/name-rev.c    | 2 +-
 builtin/show-branch.c | 4 ++--
 commit.c              | 2 +-
 fetch-pack.c          | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 992a79c..3b8564c 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1801,7 +1801,7 @@ static void assign_blame(struct scoreboard *sb, int opt)
 		 * so hold onto it in the meantime.
 		 */
 		origin_incref(suspect);
-		parse_commit(commit);
+		parse_commit_or_die(commit);
 		if (reverse ||
 		    (!(commit->object.flags & UNINTERESTING) &&
 		     !(revs->max_age != -1 && commit->date < revs->max_age)))
diff --git a/builtin/describe.c b/builtin/describe.c
index 01490a1..8299b16 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -199,7 +199,7 @@ static unsigned long finish_depth_computation(
 			best->depth++;
 		while (parents) {
 			struct commit *p = parents->item;
-			parse_commit(p);
+			parse_commit_or_die(p);
 			if (!(p->object.flags & SEEN))
 				commit_list_insert_by_date(p, list);
 			p->object.flags |= c->object.flags;
@@ -322,7 +322,7 @@ static void describe(const char *arg, int last_one)
 		}
 		while (parents) {
 			struct commit *p = parents->item;
-			parse_commit(p);
+			parse_commit_or_die(p);
 			if (!(p->object.flags & SEEN))
 				commit_list_insert_by_date(p, &list);
 			p->object.flags |= c->object.flags;
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index cd89d48..92c3316 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -29,7 +29,7 @@ static void name_rev(struct commit *commit,
 	struct commit_list *parents;
 	int parent_number = 1;
 
-	parse_commit(commit);
+	parse_commit_or_die(commit);
 
 	if (commit->date < cutoff)
 		return;
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 974f340..fd911b5 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -218,7 +218,7 @@ static void join_revs(struct commit_list **list_p,
 			parents = parents->next;
 			if ((this_flag & flags) == flags)
 				continue;
-			parse_commit(p);
+			parse_commit_or_die(p);
 			if (mark_seen(p, seen_p) && !still_interesting)
 				extra--;
 			p->object.flags |= flags;
@@ -835,7 +835,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		if (!commit)
 			die(_("cannot find commit %s (%s)"),
 			    ref_name[num_rev], oid_to_hex(&revkey));
-		parse_commit(commit);
+		parse_commit_or_die(commit);
 		mark_seen(commit, &seen);
 
 		/* rev#0 uses bit REV_SHIFT, rev#1 uses bit REV_SHIFT+1,
diff --git a/commit.c b/commit.c
index b9c0c81..5b23eaf 100644
--- a/commit.c
+++ b/commit.c
@@ -910,7 +910,7 @@ static void mark_redundant(struct commit **array, int cnt)
 	ALLOC_ARRAY(filled_index, cnt - 1);
 
 	for (i = 0; i < cnt; i++)
-		parse_commit(array[i]);
+		parse_commit_or_die(array[i]);
 	for (i = 0; i < cnt; i++) {
 		struct commit_list *common;
 
diff --git a/fetch-pack.c b/fetch-pack.c
index cb45c34..8b4ab47 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -159,7 +159,7 @@ static const unsigned char *get_rev(void)
 			return NULL;
 
 		commit = prio_queue_get(&rev_list);
-		parse_commit(commit);
+		parse_commit_or_die(commit);
 		parents = commit->parents;
 
 		commit->object.flags |= POPPED;
-- 
2.10.1.448.g862ec83.dirty

Re: Fwd: New Defects reported by Coverity Scan for git

[Stefan Beller]

On Thu, Oct 20, 2016 at 11:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Good to know that you have been managing it; I was mostly worried
> about not having anybody managing it (i.e. imagining Coverity
> nominated/volunteered me as manager with everybody else as viewers)
> and the new viewer requests get totally ignored by the project as
> the whole.

No, I have been paying attention, but in case I suddenly stop contributing
to the git project I thought it's better to have a couple of people there.

>
>> I see that some of these emails may be inconvenient to you, I can
>> change your role to defect viewer/contributor if you prefer.
>
> It is not a huge inconvenience to me, because any piece of e-mail
> that is addressed directly to gitster@ without CC'ing to git@vger
> and is not a follow-up to any earlier message goes to a separate
> lowest-priority mailbox that I rarely look at.  But if it is easy
> to recategorize me, please do so.

done.

Re: Fwd: New Defects reported by Coverity Scan for git

[Jeff King]

On Thu, Oct 20, 2016 at 10:05:38AM -0700, Stefan Beller wrote:

> Not sure what triggered the new finding of coverity as seen below as the
> parse_commit() was not touched. Junios series regarding the merge base
> optimization touches a bit of code nearby though.

I have noticed that "old" problems sometimes reappear when nearby code
changes. I assume that they have some kind of heuristic to identify the
location of a defect, that it probably includes the line number in the
file, and that it can be fooled by too much code appearing nearby.

-Peff

Re: Fwd: New Defects reported by Coverity Scan for git

[Jeff King]

On Thu, Oct 20, 2016 at 10:58:39AM -0700, Stefan Beller wrote:

> > By the way, do you know who is managing the service on our end
> > (e.g. approving new people to be "defect viewer")?
> 
> I do it most of the time, but I did not start managing it.
> And I have been pretty lax/liberal about handing out rights to do stuff,
> because I did not want to tip on anyone's toes giving to few rights
> and thereby annoying them.

I also do this, though I admit with more urgency when I recognize the
name as somebody who has showed up on the git list.

I wish there was a flag to simply make the results public. There is no
point in anybody logging in just to view them, but I don't think
Coverity makes that an option.

-Peff

Re: Fwd: New Defects reported by Coverity Scan for git

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Oct 20, 2016 at 10:58:39AM -0700, Stefan Beller wrote:
>
>> > By the way, do you know who is managing the service on our end
>> > (e.g. approving new people to be "defect viewer")?
>> 
>> I do it most of the time, but I did not start managing it.
>> And I have been pretty lax/liberal about handing out rights to do stuff,
>> because I did not want to tip on anyone's toes giving to few rights
>> and thereby annoying them.
>
> I also do this, though I admit with more urgency when I recognize the
> name as somebody who has showed up on the git list.

Well, then huge thanks to both of you.

Fwd: New Defects reported by Coverity Scan for git

[Stefan Beller]

I looked at this report for a while. My current understanding:
* its detection was triggered by including rs/move-array,
  f331ab9d4c (use MOVE_ARRAY, 2017-07-15)
* But it is harmless, because the scan logic does not understand
  how ALLOC_GROW works. It assumes that
  done_pbase_paths_alloc can be larger
  than done_pbase_paths_num + 1, while done_pbase_paths
  is NULL, such that the memory allocation is not triggered.
  If that were the case, then we have 2 subsequent dereferences
  of a NULL pointer right after that. But by inspecting the use
  of _alloc and _num the initial assumption does not seem possible.

Stefan

---------- Forwarded message ----------
From:  <scan-admin@coverity.com>
Date: Tue, Jul 18, 2017 at 2:53 AM
Subject: New Defects reported by Coverity Scan for git
To: sbeller@google.com



Hi,

Please find the latest report on new defect(s) introduced to git found
with Coverity Scan.

2 new defect(s) introduced to git found with Coverity Scan.
1 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.

New defect(s) Reported-by: Coverity Scan
Showing 2 of 2 defect(s)


** CID 1415508:  Null pointer dereferences  (FORWARD_NULL)
/builtin/pack-objects.c: 1292 in check_pbase_path()


________________________________________________________________________________________________________
*** CID 1415508:  Null pointer dereferences  (FORWARD_NULL)
/builtin/pack-objects.c: 1292 in check_pbase_path()
1286            }
1287            return -lo-1;
1288     }
1289
1290     static int check_pbase_path(unsigned hash)
1291     {
>>>     CID 1415508:  Null pointer dereferences  (FORWARD_NULL)
>>>     Comparing "done_pbase_paths" to null implies that "done_pbase_paths" might be null.
1292            int pos = (!done_pbase_paths) ? -1 : done_pbase_path_pos(hash);
1293            if (0 <= pos)
1294                    return 1;
1295            pos = -pos - 1;
1296            ALLOC_GROW(done_pbase_paths,
1297                       done_pbase_paths_num + 1,

** CID 1415507:  Null pointer dereferences  (FORWARD_NULL)
/builtin/pack-objects.c: 1303 in check_pbase_path()


________________________________________________________________________________________________________
*** CID 1415507:  Null pointer dereferences  (FORWARD_NULL)
/builtin/pack-objects.c: 1303 in check_pbase_path()
1297                       done_pbase_paths_num + 1,
1298                       done_pbase_paths_alloc);
1299            done_pbase_paths_num++;
1300            if (pos < done_pbase_paths_num)
1301                    MOVE_ARRAY(done_pbase_paths + pos + 1,
done_pbase_paths + pos,
1302                               done_pbase_paths_num - pos - 1);
>>>     CID 1415507:  Null pointer dereferences  (FORWARD_NULL)
>>>     Dereferencing null pointer "done_pbase_paths".
1303            done_pbase_paths[pos] = hash;
1304            return 0;
1305     }
1306
1307     static void add_preferred_base_object(const char *name)
1308     {


________________________________________________________________________________________________________
To view the defects in Coverity Scan visit,
https://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRb8HAP5hlBzHe8sORKm64S-2F81GsNbRdSrOteP-2FXoviMkw-3D-3D_PwiGIFugGOKqSZ6DZhASdI2SvWKInry4kHBXrJUc9pnRRRwN8fPiR-2BR4LTK2qB-2F8DwbeZJjY7Zg2FBdb8jgiAk7m6rh1YdNCvPYCPUewgRcPRcmkOFDltPB2GLYjg5Pl86kCKSRkx6inI-2BuknVr53Cjba4HgtlWmCuW5A0WMiIFvSKDW3-2BKYfPjiZDMCOFSGSLivQrUyaTeOHAHjl-2FNvbw-3D-3D

To manage Coverity Scan email notifications for "sbeller@google.com",
click https://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRbVDbis712qZDP-2FA8y06Nq4rtNFBzV5kav4CghkcEfRxSYnY6rsKHvgCYp1ThXvyV0VWbGuKIDENjx2sj6ivdYZu-2BNbJM6lgB1oY5D28iuW580xRVIt7xUSma4mf0o8-2BwE-3D_PwiGIFugGOKqSZ6DZhASdI2SvWKInry4kHBXrJUc9pnRRRwN8fPiR-2BR4LTK2qB-2F8ec7P8LTccgviKTLC0eUY7vUYOHaxCJX7GTQpS8ooD-2BtrxVu-2BilxPyHEoqsJLDaUcr6ObouH5nHR8K0ccYTKk6yC1yT-2BgMwWml4OIILno46DqjVrTy1kpeg4B-2BRv4QBTs54v6KZ4s-2FPtTLU3-2BsF7qgg-3D-3D

Re: Fwd: New Defects reported by Coverity Scan for git

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> I looked at this report for a while. My current understanding:
> * its detection was triggered by including rs/move-array,
>   f331ab9d4c (use MOVE_ARRAY, 2017-07-15)
> * But it is harmless, because the scan logic does not understand
>   how ALLOC_GROW works. It assumes that
>   done_pbase_paths_alloc can be larger
>   than done_pbase_paths_num + 1, while done_pbase_paths
>   is NULL, such that the memory allocation is not triggered.
>   If that were the case, then we have 2 subsequent dereferences
>   of a NULL pointer right after that. But by inspecting the use
>   of _alloc and _num the initial assumption does not seem possible.

Yes, it does appear that way.  ALLOC_GROW() calls REALLOC_ARRAY()
which safely can realloc NULL to specified size via xrealloc().

I'd be more worried about segfault we seem to be getting only on
Windows from this:

    git -C parent grep -e "(1|2)d(3|4)" --recurse-submodules HEAD^ > actual

in https://travis-ci.org/git/git/jobs/254654195 by the way.

Re: Fwd: New Defects reported by Coverity Scan for git

[Stefan Beller]

On Tue, Jul 18, 2017 at 10:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> I looked at this report for a while. My current understanding:
>> * its detection was triggered by including rs/move-array,
>>   f331ab9d4c (use MOVE_ARRAY, 2017-07-15)
>> * But it is harmless, because the scan logic does not understand
>>   how ALLOC_GROW works. It assumes that
>>   done_pbase_paths_alloc can be larger
>>   than done_pbase_paths_num + 1, while done_pbase_paths
>>   is NULL, such that the memory allocation is not triggered.
>>   If that were the case, then we have 2 subsequent dereferences
>>   of a NULL pointer right after that. But by inspecting the use
>>   of _alloc and _num the initial assumption does not seem possible.
>
> Yes, it does appear that way.  ALLOC_GROW() calls REALLOC_ARRAY()
> which safely can realloc NULL to specified size via xrealloc().
>
> I'd be more worried about segfault we seem to be getting only on
> Windows from this:
>
>     git -C parent grep -e "(1|2)d(3|4)" --recurse-submodules HEAD^ > actual
>
> in https://travis-ci.org/git/git/jobs/254654195 by the way.

Thanks for bringing that to my attention, (I do not follow the travis builds
as closely, as there is no yelling email in my inbox).

Windows builds on travis seem to be flaky.
(sometimes they do not start), but there are also
successful builds, including the -rc0, which may indicate
bw/grep-recurse-submodules may be faulty on Windows.

Re: Fwd: New Defects reported by Coverity Scan for git

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> I'd be more worried about segfault we seem to be getting only on
>> Windows from this:
>>
>>     git -C parent grep -e "(1|2)d(3|4)" --recurse-submodules HEAD^ > actual
>>
>> in https://travis-ci.org/git/git/jobs/254654195 by the way.
>
> Thanks for bringing that to my attention, (I do not follow the travis builds
> as closely, as there is no yelling email in my inbox).
>
> Windows builds on travis seem to be flaky.
> (sometimes they do not start), but there are also
> successful builds, including the -rc0, which may indicate
> bw/grep-recurse-submodules may be faulty on Windows.

I can get valgrind complaints locally from

    $ cd t && sh t7814-grep*.sh --valgrind -i -v

so this may not be Windows only.  Can repo_worktree_path() return a NULL
in repo_read_gitmodules() to cause git_config_from_file() barf on a NULL
gitmodule_path?

==20383== Syscall param open(filename) points to unaddressable byte(s)
==20383==    at 0x5153140: __open_nocancel (/build/eglibc-SvCtMH/eglibc-2.19/io/../sysdeps/unix/syscall-template.S:81)
==20383==    by 0x50DDED7: _IO_file_fopen@@GLIBC_2.2.5 (/build/eglibc-SvCtMH/eglibc-2.19/libio/fileops.c:228)
==20383==    by 0x50D23D3: __fopen_internal (/build/eglibc-SvCtMH/eglibc-2.19/libio/iofopen.c:90)
==20383==    by 0x569107: git_fopen (/home/gitster/git.git/compat/fopen.c:22)
==20383==    by 0x55B1ED: fopen_or_warn (/home/gitster/git.git/wrapper.c:439)
==20383==    by 0x4A2A32: git_config_from_file (/home/gitster/git.git/config.c:1442)
==20383==    by 0x540317: repo_read_gitmodules (/home/gitster/git.git/submodule.c:269)
==20383==    by 0x434389: grep_submodule (/home/gitster/git.git/builtin/grep.c:422)
==20383==    by 0x4348C8: grep_tree (/home/gitster/git.git/builtin/grep.c:580)
==20383==    by 0x434867: grep_tree (/home/gitster/git.git/builtin/grep.c:576)
==20383==    by 0x436034: cmd_grep (/home/gitster/git.git/builtin/grep.c:622)
==20383==    by 0x4063DC: handle_builtin (/home/gitster/git.git/git.c:330)
==20383==  Address 0x0 is not stack'd, malloc'd or (recently) free'd

Re: Fwd: New Defects reported by Coverity Scan for git

[Brandon Williams]

On 07/18, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
> 
> >> I'd be more worried about segfault we seem to be getting only on
> >> Windows from this:
> >>
> >>     git -C parent grep -e "(1|2)d(3|4)" --recurse-submodules HEAD^ > actual
> >>
> >> in https://travis-ci.org/git/git/jobs/254654195 by the way.
> >
> > Thanks for bringing that to my attention, (I do not follow the travis builds
> > as closely, as there is no yelling email in my inbox).
> >
> > Windows builds on travis seem to be flaky.
> > (sometimes they do not start), but there are also
> > successful builds, including the -rc0, which may indicate
> > bw/grep-recurse-submodules may be faulty on Windows.
> 
> I can get valgrind complaints locally from
> 
>     $ cd t && sh t7814-grep*.sh --valgrind -i -v
> 
> so this may not be Windows only.  Can repo_worktree_path() return a NULL
> in repo_read_gitmodules() to cause git_config_from_file() barf on a NULL
> gitmodule_path?

Yep that's most likely the cause.  The issue is if a repository doesn't
have a worktree then what should a worktree path look like?
repo_read_gitmodules() should check if there is a worktree before trying
to load the gitmodules file.  I actually noticed this and have it fixed locally in
another series I'm working on right now.  Looks like I may have to get
that change in first though.  Thanks for finding this.

> 
> ==20383== Syscall param open(filename) points to unaddressable byte(s)
> ==20383==    at 0x5153140: __open_nocancel (/build/eglibc-SvCtMH/eglibc-2.19/io/../sysdeps/unix/syscall-template.S:81)
> ==20383==    by 0x50DDED7: _IO_file_fopen@@GLIBC_2.2.5 (/build/eglibc-SvCtMH/eglibc-2.19/libio/fileops.c:228)
> ==20383==    by 0x50D23D3: __fopen_internal (/build/eglibc-SvCtMH/eglibc-2.19/libio/iofopen.c:90)
> ==20383==    by 0x569107: git_fopen (/home/gitster/git.git/compat/fopen.c:22)
> ==20383==    by 0x55B1ED: fopen_or_warn (/home/gitster/git.git/wrapper.c:439)
> ==20383==    by 0x4A2A32: git_config_from_file (/home/gitster/git.git/config.c:1442)
> ==20383==    by 0x540317: repo_read_gitmodules (/home/gitster/git.git/submodule.c:269)
> ==20383==    by 0x434389: grep_submodule (/home/gitster/git.git/builtin/grep.c:422)
> ==20383==    by 0x4348C8: grep_tree (/home/gitster/git.git/builtin/grep.c:580)
> ==20383==    by 0x434867: grep_tree (/home/gitster/git.git/builtin/grep.c:576)
> ==20383==    by 0x436034: cmd_grep (/home/gitster/git.git/builtin/grep.c:622)
> ==20383==    by 0x4063DC: handle_builtin (/home/gitster/git.git/git.c:330)
> ==20383==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
> 
> 

-- 
Brandon Williams

Re: Fwd: New Defects reported by Coverity Scan for git

[Stefan Beller]

On Tue, Jul 18, 2017 at 11:00 AM, Brandon Williams <bmwill@google.com> wrote:
> On 07/18, Junio C Hamano wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>> >> I'd be more worried about segfault we seem to be getting only on
>> >> Windows from this:
>> >>
>> >>     git -C parent grep -e "(1|2)d(3|4)" --recurse-submodules HEAD^ > actual
>> >>
>> >> in https://travis-ci.org/git/git/jobs/254654195 by the way.
>> >
>> > Thanks for bringing that to my attention, (I do not follow the travis builds
>> > as closely, as there is no yelling email in my inbox).
>> >
>> > Windows builds on travis seem to be flaky.
>> > (sometimes they do not start), but there are also
>> > successful builds, including the -rc0, which may indicate
>> > bw/grep-recurse-submodules may be faulty on Windows.
>>
>> I can get valgrind complaints locally from
>>
>>     $ cd t && sh t7814-grep*.sh --valgrind -i -v
>>
>> so this may not be Windows only.  Can repo_worktree_path() return a NULL
>> in repo_read_gitmodules() to cause git_config_from_file() barf on a NULL
>> gitmodule_path?
>
> Yep that's most likely the cause.  The issue is if a repository doesn't
> have a worktree then what should a worktree path look like?
> repo_read_gitmodules() should check if there is a worktree before trying
> to load the gitmodules file.  I actually noticed this and have it fixed locally in
> another series I'm working on right now.  Looks like I may have to get
> that change in first though.  Thanks for finding this.

If there is no worktree, we could fallback to read it from the tree
HEAD:.gitmodules, if that doesn't exist, then there are no submodules.

Thanks,
Stefan

Re: Fwd: New Defects reported by Coverity Scan for git

[René Scharfe]

Am 18.07.2017 um 19:23 schrieb Junio C Hamano:
> Stefan Beller <sbeller@google.com> writes:
> 
>> I looked at this report for a while. My current understanding:
>> * its detection was triggered by including rs/move-array,
>>    f331ab9d4c (use MOVE_ARRAY, 2017-07-15)
>> * But it is harmless, because the scan logic does not understand
>>    how ALLOC_GROW works. It assumes that
>>    done_pbase_paths_alloc can be larger
>>    than done_pbase_paths_num + 1, while done_pbase_paths
>>    is NULL, such that the memory allocation is not triggered.
>>    If that were the case, then we have 2 subsequent dereferences
>>    of a NULL pointer right after that. But by inspecting the use
>>    of _alloc and _num the initial assumption does not seem possible.
> 
> Yes, it does appear that way.  ALLOC_GROW() calls REALLOC_ARRAY()
> which safely can realloc NULL to specified size via xrealloc().

MOVE_ARRAY is passing its pointer arguments to memmove(); all it adds
is a check for (done_pbase_paths_num - pos - 1) being zero.  I don't
understand how that change can make it more likely for one of the
pointers to be NULL.

I guess the first message ('Comparing "done_pbase_paths" to null
implies that "done_pbase_paths" might be null.') has to be understood
as an explanation of how the checker arrived at the second one?

We could remove that NULL check -- it's effectively just a shortcut.
But how would that improve safety?  Well, if the array is unallocated
(NULL) and _num is greater than zero we'd get a segfault without it,
and thus would notice it.  That check currently papers over such a
hypothetical bug.  Makes sense?

-- >8 --
Subject: [PATCH] pack-objects: remove unnecessary NULL check

If done_pbase_paths is NULL then done_pbase_paths_num must be zero and
done_pbase_path_pos() returns -1 without accessing the array, so the
check is not necessary.

If the invariant was violated then the check would make sure we keep
on going and allocate the necessary amount of memory in the next
ALLOC_GROW call.  That sounds nice, but all array entries except for
one would contain garbage data.

If the invariant was violated without the check we'd get a segfault in
done_pbase_path_pos(), i.e. an observable crash, alerting us of the
presence of a bug.

Currently there is no such bug: Only the functions check_pbase_path()
and cleanup_preferred_base() change pointer and counter, and both make
sure to keep them in sync.  Get rid of the check anyway to allow us to
see if later changes introduce such a defect, and to simplify the code.

Detected by Coverity Scan.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/pack-objects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index e730b415bf..c753e9237a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1289,7 +1289,7 @@ static int done_pbase_path_pos(unsigned hash)
 
 static int check_pbase_path(unsigned hash)
 {
-	int pos = (!done_pbase_paths) ? -1 : done_pbase_path_pos(hash);
+	int pos = done_pbase_path_pos(hash);
 	if (0 <= pos)
 		return 1;
 	pos = -pos - 1;
-- 
2.13.3

Re: Fwd: New Defects reported by Coverity Scan for git

[Junio C Hamano]

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

> We could remove that NULL check -- it's effectively just a shortcut.
> But how would that improve safety?  Well, if the array is unallocated
> (NULL) and _num is greater than zero we'd get a segfault without it,
> and thus would notice it.  That check currently papers over such a
> hypothetical bug.  Makes sense?
>
> -- >8 --
> Subject: [PATCH] pack-objects: remove unnecessary NULL check
>
> If done_pbase_paths is NULL then done_pbase_paths_num must be zero and
> done_pbase_path_pos() returns -1 without accessing the array, so the
> check is not necessary.
>
> If the invariant was violated then the check would make sure we keep
> on going and allocate the necessary amount of memory in the next
> ALLOC_GROW call.  That sounds nice, but all array entries except for
> one would contain garbage data.
>
> If the invariant was violated without the check we'd get a segfault in
> done_pbase_path_pos(), i.e. an observable crash, alerting us of the
> presence of a bug.
>
> Currently there is no such bug: Only the functions check_pbase_path()
> and cleanup_preferred_base() change pointer and counter, and both make
> sure to keep them in sync.  Get rid of the check anyway to allow us to
> see if later changes introduce such a defect, and to simplify the code.
>
> Detected by Coverity Scan.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---

It's always amusing to see that a removal of conditional codepath
would result in better chance of finding possible invariant
breakers, as we often think that belt-and-suspenders safety would
require more conditionals and asserts ;-)



>  builtin/pack-objects.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index e730b415bf..c753e9237a 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1289,7 +1289,7 @@ static int done_pbase_path_pos(unsigned hash)
>  
>  static int check_pbase_path(unsigned hash)
>  {
> -	int pos = (!done_pbase_paths) ? -1 : done_pbase_path_pos(hash);
> +	int pos = done_pbase_path_pos(hash);
>  	if (0 <= pos)
>  		return 1;
>  	pos = -pos - 1;

Fwd: New Defects reported by Coverity Scan for git

[Stefan Beller]

coverity scan failed for the last couple month (since Nov 20th)
without me noticing, I plan on running it again nightly for the
Git project.

Anyway, here are issues that piled up (in origin/pu) since then.

Stefan


---------- Forwarded message ----------
From:  <scan-admin@coverity.com>
Date: Mon, Mar 26, 2018 at 4:24 PM
Subject: New Defects reported by Coverity Scan for git
To: sbeller@google.com


Hi,

Please find the latest report on new defect(s) introduced to git found
with Coverity Scan.

44 new defect(s) introduced to git found with Coverity Scan.
32 defect(s), reported by Coverity Scan earlier, were marked fixed in
the recent build analyzed by Coverity Scan.

New defect(s) Reported-by: Coverity Scan
Showing 20 of 44 defect(s)


** CID 1433546:  Resource leaks  (RESOURCE_LEAK)
/t/helper/test-path-utils.c: 236 in cmd__path_utils()


________________________________________________________________________________________________________
*** CID 1433546:  Resource leaks  (RESOURCE_LEAK)
/t/helper/test-path-utils.c: 236 in cmd__path_utils()
230             if (argc >= 4 && !strcmp(argv[1], "prefix_path")) {
231                     const char *prefix = argv[2];
232                     int prefix_len = strlen(prefix);
233                     int nongit_ok;
234                     setup_git_directory_gently(&nongit_ok);
235                     while (argc > 3) {
>>>     CID 1433546:  Resource leaks  (RESOURCE_LEAK)
>>>     Failing to save or free storage allocated by "prefix_path(prefix, prefix_len, argv[3])" leaks it.
236                             puts(prefix_path(prefix, prefix_len, argv[3]));
237                             argc--;
238                             argv++;
239                     }
240                     return 0;
241             }

** CID 1433545:  Security best practices violations  (STRING_OVERFLOW)
/merge-recursive.c: 1955 in check_dir_renamed()


________________________________________________________________________________________________________
*** CID 1433545:  Security best practices violations  (STRING_OVERFLOW)
/merge-recursive.c: 1955 in check_dir_renamed()
1949                                                      struct
hashmap *dir_renames)
1950     {
1951            char temp[PATH_MAX];
1952            char *end;
1953            struct dir_rename_entry *entry;
1954
>>>     CID 1433545:  Security best practices violations  (STRING_OVERFLOW)
>>>     You might overrun the 4096-character fixed-size string "temp" by copying "path" without checking the length.
1955            strcpy(temp, path);
1956            while ((end = strrchr(temp, '/'))) {
1957                    *end = '\0';
1958                    entry = dir_rename_find_entry(dir_renames, temp);
1959                    if (entry)
1960                            return entry;

** CID 1433544:  Resource leaks  (RESOURCE_LEAK)
/builtin/submodule--helper.c: 66 in print_default_remote()


________________________________________________________________________________________________________
*** CID 1433544:  Resource leaks  (RESOURCE_LEAK)
/builtin/submodule--helper.c: 66 in print_default_remote()
60              die(_("submodule--helper print-default-remote takes no
arguments"));
61
62      remote = get_default_remote();
63      if (remote)
64              printf("%s\n", remote);
65
>>>     CID 1433544:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "remote" going out of scope leaks the storage it points to.
66      return 0;
67     }
68
69     static int starts_with_dot_slash(const char *str)
70     {
71      return str[0] == '.' && is_dir_sep(str[1]);

** CID 1433543:  Null pointer dereferences  (NULL_RETURNS)
/merge-recursive.c: 812 in was_dirty()


________________________________________________________________________________________________________
*** CID 1433543:  Null pointer dereferences  (NULL_RETURNS)
/merge-recursive.c: 812 in was_dirty()
806             int dirty = 1;
807
808             if (o->call_depth || !was_tracked(path))
809                     return !dirty;
810
811             ce = cache_file_exists(path, strlen(path), ignore_case);
>>>     CID 1433543:  Null pointer dereferences  (NULL_RETURNS)
>>>     Dereferencing a null pointer "ce".
812             dirty = (ce->ce_stat_data.sd_mtime.sec > 0 &&
813                      verify_uptodate(ce, &o->unpack_opts) != 0);
814             return dirty;
815     }
816
817     static int make_room_for_path(struct merge_options *o, const char *path)

** CID 1433542:  Error handling issues  (CHECKED_RETURN)
/merge-recursive.c: 2162 in apply_directory_rename_modifications()


________________________________________________________________________________________________________
*** CID 1433542:  Error handling issues  (CHECKED_RETURN)
/merge-recursive.c: 2162 in apply_directory_rename_modifications()
2156             * "NOTE" in update_stages(), doing so will modify the current
2157             * in-memory index which will break calls to
would_lose_untracked()
2158             * that we need to make.  Instead, we need to just
make sure that
2159             * the various conflict_rename_*() functions update the index
2160             * explicitly rather than relying on unpack_trees() to
have done it.
2161             */
>>>     CID 1433542:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "get_tree_entry" without checking return value (as is done elsewhere 13 out of 16 times).
2162            get_tree_entry(&tree->object.oid,
2163                           pair->two->path,
2164                           &re->dst_entry->stages[stage].oid,
2165                           &re->dst_entry->stages[stage].mode);
2166
2167            /* Update pair status */

** CID 1433541:  Resource leaks  (RESOURCE_LEAK)
/t/helper/test-path-utils.c: 246 in cmd__path_utils()


________________________________________________________________________________________________________
*** CID 1433541:  Resource leaks  (RESOURCE_LEAK)
/t/helper/test-path-utils.c: 246 in cmd__path_utils()
240                     return 0;
241             }
242
243             if (argc == 4 && !strcmp(argv[1], "strip_path_suffix")) {
244                     char *prefix = strip_path_suffix(argv[2], argv[3]);
245                     printf("%s\n", prefix ? prefix : "(null)");
>>>     CID 1433541:  Resource leaks  (RESOURCE_LEAK)
>>>     Variable "prefix" going out of scope leaks the storage it points to.
246                     return 0;
247             }
248
249             if (argc == 3 && !strcmp(argv[1], "print_path")) {
250                     puts(argv[2]);
251                     return 0;

** CID 1433540:  Null pointer dereferences  (REVERSE_INULL)
/upload-pack.c: 834 in process_deepen_since()


________________________________________________________________________________________________________
*** CID 1433540:  Null pointer dereferences  (REVERSE_INULL)
/upload-pack.c: 834 in process_deepen_since()
828     static int process_deepen_since(const char *line, timestamp_t
*deepen_since, int *deepen_rev_list)
829     {
830             const char *arg;
831             if (skip_prefix(line, "deepen-since ", &arg)) {
832                     char *end = NULL;
833                     *deepen_since = parse_timestamp(arg, &end, 0);
>>>     CID 1433540:  Null pointer dereferences  (REVERSE_INULL)
>>>     Null-checking "deepen_since" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
834                     if (!end || *end || !deepen_since ||
835                         /* revisions.c's max_age -1 is special */
836                         *deepen_since == -1)
837                             die("Invalid deepen-since: %s", line);
838                     *deepen_rev_list = 1;
839                     return 1;

** CID 1433539:  Null pointer dereferences  (FORWARD_NULL)


________________________________________________________________________________________________________
*** CID 1433539:  Null pointer dereferences  (FORWARD_NULL)
/t/helper/test-json-writer.c: 278 in scripted()
272             struct json_writer jw = JSON_WRITER_INIT;
273             int k;
274
275             if (!strcmp(argv[0], "@object"))
276                     jw_object_begin(&jw);
277             else if (!strcmp(argv[0], "@array"))
>>>     CID 1433539:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing "&jw" to "jw_array_begin", which dereferences null "jw.levels".
278                     jw_array_begin(&jw);
279             else
280                     die("first script term must be '@object' or
'@array': '%s'", argv[0]);
281
282             for (k = 1; k < argc; k++) {
283                     const char *a_k = argv[k];

** CID 1433538:  Null pointer dereferences  (FORWARD_NULL)


________________________________________________________________________________________________________
*** CID 1433538:  Null pointer dereferences  (FORWARD_NULL)
/t/helper/test-sha1-array.c: 23 in cmd__sha1_array()
17              const char *arg;
18              struct object_id oid;
19
20              if (skip_prefix(line.buf, "append ", &arg)) {
21                      if (get_oid_hex(arg, &oid))
22                              die("not a hexadecimal SHA1: %s", arg);
>>>     CID 1433538:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing "&array" to "oid_array_append", which dereferences null "array.oid".
23                      oid_array_append(&array, &oid);
24              } else if (skip_prefix(line.buf, "lookup ", &arg)) {
25                      if (get_oid_hex(arg, &oid))
26                              die("not a hexadecimal SHA1: %s", arg);
27                      printf("%d\n", oid_array_lookup(&array, &oid));
28              } else if (!strcmp(line.buf, "clear"))

** CID 1433537:    (FORWARD_NULL)
/merge-recursive.c: 1796 in handle_directory_level_conflicts()
/merge-recursive.c: 1791 in handle_directory_level_conflicts()


________________________________________________________________________________________________________
*** CID 1433537:    (FORWARD_NULL)
/merge-recursive.c: 1796 in handle_directory_level_conflicts()
1790                            strbuf_release(&head_ent->new_dir);
1791                            string_list_append(&remove_from_merge,
1792
merge_ent->dir)->util = merge_ent;
1793                            strbuf_release(&merge_ent->new_dir);
1794                    } else if (tree_has_path(head, head_ent->dir)) {
1795                            /* 2. This wasn't a directory rename
after all */
>>>     CID 1433537:    (FORWARD_NULL)
>>>     Dereferencing null pointer "string_list_append(&remove_from_head, head_ent->dir)".
1796                            string_list_append(&remove_from_head,
1797
head_ent->dir)->util = head_ent;
1798                            strbuf_release(&head_ent->new_dir);
1799                    }
1800            }
1801
/merge-recursive.c: 1791 in handle_directory_level_conflicts()
1785                        !merge_ent->non_unique_new_dir &&
1786                        !strbuf_cmp(&head_ent->new_dir,
&merge_ent->new_dir)) {
1787                            /* 1. Renamed identically; remove it
from both sides */
1788                            string_list_append(&remove_from_head,
1789
head_ent->dir)->util = head_ent;
1790                            strbuf_release(&head_ent->new_dir);
>>>     CID 1433537:    (FORWARD_NULL)
>>>     Dereferencing null pointer "string_list_append(&remove_from_merge, merge_ent->dir)".
1791                            string_list_append(&remove_from_merge,
1792
merge_ent->dir)->util = merge_ent;
1793                            strbuf_release(&merge_ent->new_dir);
1794                    } else if (tree_has_path(head, head_ent->dir)) {
1795                            /* 2. This wasn't a directory rename
after all */
1796                            string_list_append(&remove_from_head,

** CID 1433536:    (RESOURCE_LEAK)
/t/helper/test-delta.c: 34 in cmd__delta()
/t/helper/test-delta.c: 48 in cmd__delta()
/t/helper/test-delta.c: 75 in cmd__delta()
/t/helper/test-delta.c: 78 in cmd__delta()


________________________________________________________________________________________________________
*** CID 1433536:    (RESOURCE_LEAK)
/t/helper/test-delta.c: 34 in cmd__delta()
28              return 1;
29      }
30
31      fd = open(argv[2], O_RDONLY);
32      if (fd < 0 || fstat(fd, &st)) {
33              perror(argv[2]);
>>>     CID 1433536:    (RESOURCE_LEAK)
>>>     Handle variable "fd" going out of scope leaks the handle.
34              return 1;
35      }
36      from_size = st.st_size;
37      from_buf = mmap(NULL, from_size, PROT_READ, MAP_PRIVATE, fd, 0);
38      if (from_buf == MAP_FAILED) {
39              perror(argv[2]);
/t/helper/test-delta.c: 48 in cmd__delta()
42      }
43      close(fd);
44
45      fd = open(argv[3], O_RDONLY);
46      if (fd < 0 || fstat(fd, &st)) {
47              perror(argv[3]);
>>>     CID 1433536:    (RESOURCE_LEAK)
>>>     Handle variable "fd" going out of scope leaks the handle.
48              return 1;
49      }
50      data_size = st.st_size;
51      data_buf = mmap(NULL, data_size, PROT_READ, MAP_PRIVATE, fd, 0);
52      if (data_buf == MAP_FAILED) {
53              perror(argv[3]);
/t/helper/test-delta.c: 75 in cmd__delta()
69              return 1;
70      }
71
72      fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
73      if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {
74              perror(argv[4]);
>>>     CID 1433536:    (RESOURCE_LEAK)
>>>     Handle variable "fd" going out of scope leaks the handle.
75              return 1;
76      }
77
78      return 0;
/t/helper/test-delta.c: 78 in cmd__delta()
72      fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
73      if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {
74              perror(argv[4]);
75              return 1;
76      }
77
>>>     CID 1433536:    (RESOURCE_LEAK)
>>>     Handle variable "fd" going out of scope leaks the handle.
78      return 0;

** CID 1433535:  Error handling issues  (CHECKED_RETURN)
/builtin/grep.c: 491 in grep_cache()


________________________________________________________________________________________________________
*** CID 1433535:  Error handling issues  (CHECKED_RETURN)
/builtin/grep.c: 491 in grep_cache()
485             int name_base_len = 0;
486             if (repo->submodule_prefix) {
487                     name_base_len = strlen(repo->submodule_prefix);
488                     strbuf_addstr(&name, repo->submodule_prefix);
489             }
490
>>>     CID 1433535:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "repo_read_index" without checking return value (as is done elsewhere 4 out of 5 times).
491             repo_read_index(repo);
492
493             for (nr = 0; nr < repo->index->cache_nr; nr++) {
494                     const struct cache_entry *ce = repo->index->cache[nr];
495                     strbuf_setlen(&name, name_base_len);
496                     strbuf_addstr(&name, ce->name);

** CID 1433534:  Resource leaks  (RESOURCE_LEAK)
/t/helper/test-mktemp.c: 12 in cmd__mktemp()


________________________________________________________________________________________________________
*** CID 1433534:  Resource leaks  (RESOURCE_LEAK)
/t/helper/test-mktemp.c: 12 in cmd__mktemp()
6
7     int cmd__mktemp(int argc, const char **argv)
8     {
9       if (argc != 2)
10              usage("Expected 1 parameter defining the temporary
file template");
11
>>>     CID 1433534:  Resource leaks  (RESOURCE_LEAK)
>>>     Failing to save or free storage allocated by "xstrdup(argv[1])" leaks it.
12      xmkstemp(xstrdup(argv[1]));
13
14      return 0;

** CID 1433533:    (RESOURCE_LEAK)
/t/helper/test-delta.c: 48 in cmd__delta()
/t/helper/test-delta.c: 55 in cmd__delta()
/t/helper/test-delta.c: 69 in cmd__delta()
/t/helper/test-delta.c: 75 in cmd__delta()
/t/helper/test-delta.c: 78 in cmd__delta()


________________________________________________________________________________________________________
*** CID 1433533:    (RESOURCE_LEAK)
/t/helper/test-delta.c: 48 in cmd__delta()
42      }
43      close(fd);
44
45      fd = open(argv[3], O_RDONLY);
46      if (fd < 0 || fstat(fd, &st)) {
47              perror(argv[3]);
>>>     CID 1433533:    (RESOURCE_LEAK)
>>>     Variable "from_buf" going out of scope leaks the storage it points to.
48              return 1;
49      }
50      data_size = st.st_size;
51      data_buf = mmap(NULL, data_size, PROT_READ, MAP_PRIVATE, fd, 0);
52      if (data_buf == MAP_FAILED) {
53              perror(argv[3]);
/t/helper/test-delta.c: 55 in cmd__delta()
49      }
50      data_size = st.st_size;
51      data_buf = mmap(NULL, data_size, PROT_READ, MAP_PRIVATE, fd, 0);
52      if (data_buf == MAP_FAILED) {
53              perror(argv[3]);
54              close(fd);
>>>     CID 1433533:    (RESOURCE_LEAK)
>>>     Variable "from_buf" going out of scope leaks the storage it points to.
55              return 1;
56      }
57      close(fd);
58
59      if (argv[1][1] == 'd')
60              out_buf = diff_delta(from_buf, from_size,
/t/helper/test-delta.c: 69 in cmd__delta()
63      else
64              out_buf = patch_delta(from_buf, from_size,
65                                    data_buf, data_size,
66                                    &out_size);
67      if (!out_buf) {
68              fprintf(stderr, "delta operation failed (returned NULL)\n");
>>>     CID 1433533:    (RESOURCE_LEAK)
>>>     Variable "from_buf" going out of scope leaks the storage it points to.
69              return 1;
70      }
71
72      fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
73      if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {
74              perror(argv[4]);
/t/helper/test-delta.c: 75 in cmd__delta()
69              return 1;
70      }
71
72      fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
73      if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {
74              perror(argv[4]);
>>>     CID 1433533:    (RESOURCE_LEAK)
>>>     Variable "from_buf" going out of scope leaks the storage it points to.
75              return 1;
76      }
77
78      return 0;
/t/helper/test-delta.c: 78 in cmd__delta()
72      fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
73      if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {
74              perror(argv[4]);
75              return 1;
76      }
77
>>>     CID 1433533:    (RESOURCE_LEAK)
>>>     Variable "from_buf" going out of scope leaks the storage it points to.
78      return 0;

** CID 1433532:    (RESOURCE_LEAK)
/t/helper/test-delta.c: 75 in cmd__delta()
/t/helper/test-delta.c: 78 in cmd__delta()
/t/helper/test-delta.c: 75 in cmd__delta()


________________________________________________________________________________________________________
*** CID 1433532:    (RESOURCE_LEAK)
/t/helper/test-delta.c: 75 in cmd__delta()
69              return 1;
70      }
71
72      fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
73      if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {
74              perror(argv[4]);
>>>     CID 1433532:    (RESOURCE_LEAK)
>>>     Variable "out_buf" going out of scope leaks the storage it points to.
75              return 1;
76      }
77
78      return 0;
/t/helper/test-delta.c: 78 in cmd__delta()
72      fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
73      if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {
74              perror(argv[4]);
75              return 1;
76      }
77
>>>     CID 1433532:    (RESOURCE_LEAK)
>>>     Variable "out_buf" going out of scope leaks the storage it points to.
78      return 0;
/t/helper/test-delta.c: 75 in cmd__delta()
69              return 1;
70      }
71
72      fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
73      if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {
74              perror(argv[4]);
>>>     CID 1433532:    (RESOURCE_LEAK)
>>>     Variable "out_buf" going out of scope leaks the storage it points to.
75              return 1;
76      }
77
78      return 0;

** CID 1433531:  Null pointer dereferences  (FORWARD_NULL)


________________________________________________________________________________________________________
*** CID 1433531:  Null pointer dereferences  (FORWARD_NULL)
/builtin/submodule--helper.c: 1045 in module_deinit()
1039                                       module_deinit_options);
1040            }
1041
1042            if (!argc && !all)
1043                    die(_("Use '--all' if you really want to
deinitialize all submodules"));
1044
>>>     CID 1433531:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing "&list" to "module_list_compute", which dereferences null "list.entries".
1045            if (module_list_compute(argc, argv, prefix, &pathspec,
&list) < 0)
1046                    BUG("module_list_compute should not choke on
empty pathspec");
1047
1048            info.prefix = prefix;
1049            if (quiet)
1050                    info.flags |= OPT_QUIET;

** CID 1433530:    (FORWARD_NULL)


________________________________________________________________________________________________________
*** CID 1433530:    (FORWARD_NULL)
/run-command.c: 574 in trace_add_env()
568             for (e = deltaenv; e && *e; e++) {
569                     struct strbuf key = STRBUF_INIT;
570                     char *equals = strchr(*e, '=');
571
572                     if (equals) {
573                             strbuf_add(&key, *e, equals - *e);
>>>     CID 1433530:    (FORWARD_NULL)
>>>     Passing "&envs" to "string_list_insert", which dereferences null "envs.items".
574                             string_list_insert(&envs,
key.buf)->util = equals + 1;
575                     } else {
576                             string_list_insert(&envs, *e)->util = NULL;
577                     }
578                     strbuf_release(&key);
579             }
/run-command.c: 576 in trace_add_env()
570                     char *equals = strchr(*e, '=');
571
572                     if (equals) {
573                             strbuf_add(&key, *e, equals - *e);
574                             string_list_insert(&envs,
key.buf)->util = equals + 1;
575                     } else {
>>>     CID 1433530:    (FORWARD_NULL)
>>>     Passing "&envs" to "string_list_insert", which dereferences null "envs.items".
576                             string_list_insert(&envs, *e)->util = NULL;
577                     }
578                     strbuf_release(&key);
579             }
580
581             /* "unset X Y...;" */

** CID 1433529:  Control flow issues  (DEADCODE)
/upload-pack.c: 1419 in upload_pack_v2()


________________________________________________________________________________________________________
*** CID 1433529:  Control flow issues  (DEADCODE)
/upload-pack.c: 1419 in upload_pack_v2()
1413                            send_shallow_info(&data);
1414
1415                            packet_write_fmt(1, "packfile\n");
1416                            create_pack_file();
1417                            state = FETCH_DONE;
1418                            break;
>>>     CID 1433529:  Control flow issues  (DEADCODE)
>>>     Execution cannot reach this statement: "case FETCH_DONE:".
1419                    case FETCH_DONE:
1420                            continue;
1421                    }
1422            }
1423
1424            upload_pack_data_clear(&data);

** CID 1433528:  Null pointer dereferences  (FORWARD_NULL)


________________________________________________________________________________________________________
*** CID 1433528:  Null pointer dereferences  (FORWARD_NULL)
/convert.c: 411 in encode_to_git()
405              * the content. Let's answer with "yes", since an encoding was
406              * specified.
407              */
408             if (!buf && !src)
409                     return 1;
410
>>>     CID 1433528:  Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "src" to "validate_encoding", which dereferences it.
411             if (validate_encoding(path, enc, src, src_len, die_on_error))
412                     return 0;
413
414             trace_encoding("source", path, enc, src, src_len);
415             dst = reencode_string_len(src, src_len, default_encoding, enc,
416                                       &dst_len);

** CID 1433527:  Control flow issues  (DEADCODE)
/fetch-pack.c: 1396 in do_fetch_pack_v2()


________________________________________________________________________________________________________
*** CID 1433527:  Control flow issues  (DEADCODE)
/fetch-pack.c: 1396 in do_fetch_pack_v2()
1390                            process_section_header(&reader, "packfile", 0);
1391                            if (get_pack(args, fd, pack_lockfile))
1392                                    die(_("git fetch-pack: fetch failed."));
1393
1394                            state = FETCH_DONE;
1395                            break;
>>>     CID 1433527:  Control flow issues  (DEADCODE)
>>>     Execution cannot reach this statement: "case FETCH_DONE:".
1396                    case FETCH_DONE:
1397                            continue;
1398                    }
1399            }
1400
1401            oidset_clear(&common);


________________________________________________________________________________________________________
To view the defects in Coverity Scan visit,
https://u2389337.ct.sendgrid.net/wf/click?upn=08onrYu34A-2BWcWUl-2F-2BfV0V05UPxvVjWch-2Bd2MGckcRb8HAP5hlBzHe8sORKm64S-2F81GsNbRdSrOteP-2FXoviMkw-3D-3D_PwiGIFugGOKqSZ6DZhASdI2SvWKInry4kHBXrJUc9pmXl6RPFKKio5QDumyeOncb-2B03DyHottfRb-2BR0vAZZ-2BouFuqkpeG83Y-2BMRBesAVhj5GrGot1mbZe20ytg0ii7TqV60O843zCVEbFTCr2Fj7-2Byv7sYi9qWRfEPYF5wF-2BAudpsWONlHafz3S2f-2F0Lk0mNQz1ZEGGEFx2qj7TNc4JugAxaJzb7JKTcIW0OPzH-2BQxU-3D

Re: Fwd: New Defects reported by Coverity Scan for git

[Jeff Hostetler]

On 3/26/2018 7:39 PM, Stefan Beller wrote:
> coverity scan failed for the last couple month (since Nov 20th)
> without me noticing, I plan on running it again nightly for the
> Git project.
> 
> Anyway, here are issues that piled up (in origin/pu) since then.
> 
> Stefan
> 
> 
> ---------- Forwarded message ----------
[...]
________________________________________________________________________________________________________
> *** CID 1433539:  Null pointer dereferences  (FORWARD_NULL)
> /t/helper/test-json-writer.c: 278 in scripted()
> 272             struct json_writer jw = JSON_WRITER_INIT;
> 273             int k;
> 274
> 275             if (!strcmp(argv[0], "@object"))
> 276                     jw_object_begin(&jw);
> 277             else if (!strcmp(argv[0], "@array"))
>>>>      CID 1433539:  Null pointer dereferences  (FORWARD_NULL)
>>>>      Passing "&jw" to "jw_array_begin", which dereferences null "jw.levels".
> 278                     jw_array_begin(&jw);
> 279             else
> 280                     die("first script term must be '@object' or
> '@array': '%s'", argv[0]);
> 281
> 282             for (k = 1; k < argc; k++) {
> 283                     const char *a_k = argv[k];
> 
> ** CID 1433538:  Null pointer dereferences  (FORWARD_NULL)
> 

The "jw.levels" field has been removed in the json-writer V4 reroll,
so this isn't an issue going forward.

Thanks,
Jeff