[PATCH 00/11] Coverity fixes

[PATCH 00/11] Coverity fixes

[Johannes Schindelin via GitGitGadget]

The release window is a fine time to concentrate on bug fixes, and Coverity
can help identify bugs. These are only a couple of fixes, Coverity reported
many more, but most of the reports seemed either bogus or harmless (such as
resource leaks in test helpers).

Johannes Schindelin (11):
  mingw: avoid accessing uninitialized memory in `is_executable()`
  fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()`
  submodule--helper: avoid memory leak in `update_submodule()`
  get_oid_with_context_1(): avoid use-after-free
  submodule-config: avoid memory leak
  pack-redundant: avoid using uninitialized memory
  submodule--helper: avoid memory leak when fetching submodules
  read_index_from(): avoid memory leak
  pack-mtimes: avoid closing a bogus file descriptor
  relative_url(): fix incorrect condition
  bug_fl(): add missing `va_end()` call

 builtin/pack-redundant.c    | 1 +
 builtin/submodule--helper.c | 3 +++
 fsmonitor-settings.c        | 8 ++++++--
 object-name.c               | 6 ++++--
 pack-mtimes.c               | 3 ++-
 read-cache.c                | 6 +++---
 remote.c                    | 2 +-
 run-command.c               | 2 +-
 submodule-config.c          | 8 ++++----
 usage.c                     | 1 +
 10 files changed, 26 insertions(+), 14 deletions(-)


base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1264%2Fdscho%2Fcoverity-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1264/dscho/coverity-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1264
-- 
gitgitgadget

[PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

On Windows, we open files we suspect might be scripts, read the first
two bytes, and see whether they indicate a hash-bang line. We do not
initialize the byte _after_ those two bytes, therefore `strcmp()` is
inappropriate here.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 run-command.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index 14f17830f51..2ba38850b4d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -154,7 +154,7 @@ int is_executable(const char *name)
 		n = read(fd, buf, 2);
 		if (n == 2)
 			/* look for a she-bang */
-			if (!strcmp(buf, "#!"))
+			if (!memcmp(buf, "#!", 2))
 				st.st_mode |= S_IXUSR;
 		close(fd);
 	}
-- 
gitgitgadget

Re: [PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()`

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> On Windows, we open files we suspect might be scripts, read the first
> two bytes, and see whether they indicate a hash-bang line. We do not
> initialize the byte _after_ those two bytes, therefore `strcmp()` is
> inappropriate here.
>
> Reported by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  run-command.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This has been with us since cc3b7a97 (Windows: Make 'git help -a'
work., 2008-01-14) and apparently nobody made loud enough noises
to make us aware since then.

The fix is trivially correct, of course.

Will queue.


>
> diff --git a/run-command.c b/run-command.c
> index 14f17830f51..2ba38850b4d 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -154,7 +154,7 @@ int is_executable(const char *name)
>  		n = read(fd, buf, 2);
>  		if (n == 2)
>  			/* look for a she-bang */
> -			if (!strcmp(buf, "#!"))
> +			if (!memcmp(buf, "#!", 2))
>  				st.st_mode |= S_IXUSR;
>  		close(fd);
>  	}

Re: [PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()`

[René Scharfe]

Am 16.06.22 um 01:35 schrieb Johannes Schindelin via GitGitGadget:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> On Windows, we open files we suspect might be scripts, read the first
> two bytes, and see whether they indicate a hash-bang line. We do not
> initialize the byte _after_ those two bytes, therefore `strcmp()` is
> inappropriate here.

Hmm, but buf _is_ initialized fully?  Line 149:

        char buf[3] = { 0 };

C99 §6.7.8 21: "If there are [...] fewer characters in a string literal
used to initialize an array of known size than there are elements in the
array, the remainder of the aggregate shall be initialized implicitly
the same as objects that have static storage duration."

>
> Reported by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  run-command.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index 14f17830f51..2ba38850b4d 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -154,7 +154,7 @@ int is_executable(const char *name)
>  		n = read(fd, buf, 2);
>  		if (n == 2)
>  			/* look for a she-bang */
> -			if (!strcmp(buf, "#!"))
> +			if (!memcmp(buf, "#!", 2))
>  				st.st_mode |= S_IXUSR;
>  		close(fd);
>  	}

Re: [PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()`

[Junio C Hamano]

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

> Am 16.06.22 um 01:35 schrieb Johannes Schindelin via GitGitGadget:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> On Windows, we open files we suspect might be scripts, read the first
>> two bytes, and see whether they indicate a hash-bang line. We do not
>> initialize the byte _after_ those two bytes, therefore `strcmp()` is
>> inappropriate here.
>
> Hmm, but buf _is_ initialized fully?  Line 149:
>
>         char buf[3] = { 0 };

Ahh, yeah, that changes the landscape quite a bit.

We explicitly ask to read 2 bytes and look at the buf[] when read
says it read 2 bytes, so this is another false positive X-<.

>> diff --git a/run-command.c b/run-command.c
>> index 14f17830f51..2ba38850b4d 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -154,7 +154,7 @@ int is_executable(const char *name)
>>  		n = read(fd, buf, 2);
>>  		if (n == 2)
>>  			/* look for a she-bang */
>> -			if (!strcmp(buf, "#!"))
>> +			if (!memcmp(buf, "#!", 2))
>>  				st.st_mode |= S_IXUSR;
>>  		close(fd);
>>  	}

We can update the variable to

	char buf[2];

to match the updated code, I guess.  The fewer bytes we use on
stack, the better ;-).

Re: [PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()`

[Junio C Hamano]

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

>> Hmm, but buf _is_ initialized fully?  Line 149:
>>
>>         char buf[3] = { 0 };
>
> Ahh, yeah, that changes the landscape quite a bit.

Just for reference, this piece of code has been correct ever since
it was introduced to help.c by cc3b7a97 (Windows: Make 'git help -a'
work., 2008-01-14).  With a larger context, we can see buf[3] that
is NUL filled, which receives 2 bytes by read(), and strcmp() does
look semantically more correct than memcmp(), even though there
probably shouldn't be any correctness or performance difference.

So, strcmp -> memcmp in this case is a strict dis-improvement.  I
merged it already to 'next', but I haven't pushed the result out, so
I'll redo 'next' before I push the result out.

Thanks.

[PATCH 02/11] fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 fsmonitor-settings.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
index 658cb79da01..464424a1e92 100644
--- a/fsmonitor-settings.c
+++ b/fsmonitor-settings.c
@@ -202,11 +202,15 @@ char *fsm_settings__get_incompatible_msg(const struct repository *r,
 	case FSMONITOR_REASON_OK:
 		goto done;
 
-	case FSMONITOR_REASON_BARE:
+	case FSMONITOR_REASON_BARE: {
+		char *cwd = xgetcwd();
+
 		strbuf_addf(&msg,
 			    _("bare repository '%s' is incompatible with fsmonitor"),
-			    xgetcwd());
+			    cwd);
+		free(cwd);
 		goto done;
+	}
 
 	case FSMONITOR_REASON_ERROR:
 		strbuf_addf(&msg,
-- 
gitgitgadget

Re: [PATCH 02/11] fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()`

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Reported by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  fsmonitor-settings.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
> index 658cb79da01..464424a1e92 100644
> --- a/fsmonitor-settings.c
> +++ b/fsmonitor-settings.c
> @@ -202,11 +202,15 @@ char *fsm_settings__get_incompatible_msg(const struct repository *r,
>  	case FSMONITOR_REASON_OK:
>  		goto done;

Obviously correct, but the placement of these ...

> -	case FSMONITOR_REASON_BARE:
> +	case FSMONITOR_REASON_BARE: {
> +		char *cwd = xgetcwd();
> +
>  		strbuf_addf(&msg,
>  			    _("bare repository '%s' is incompatible with fsmonitor"),
> -			    xgetcwd());
> +			    cwd);
> +		free(cwd);
>  		goto done;
> +	}

... braces are misleading and confusing.

>  
>  	case FSMONITOR_REASON_ERROR:
>  		strbuf_addf(&msg,

[PATCH 03/11] submodule--helper: avoid memory leak in `update_submodule()`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/submodule--helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5c77dfcffee..d7b8004b933 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data)
 
 		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
 								  update_data->prefix);
+		free(prefixed_path);
+
 		next.prefix = NULL;
 		oidcpy(&next.oid, null_oid());
 		oidcpy(&next.suboid, null_oid());
-- 
gitgitgadget

Re: [PATCH 03/11] submodule--helper: avoid memory leak in `update_submodule()`

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Reported by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/submodule--helper.c | 2 ++
>  1 file changed, 2 insertions(+)


> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 5c77dfcffee..d7b8004b933 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data)
>  
>  		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
>  								  update_data->prefix);
> +		free(prefixed_path);
> +

This function has two very similar code block that computes
prefixed_path depending on the same condition, and one frees the
variable correctly while the other one (i.e. this one) forgets to do
so, which is irritating to see.

Perhaps the whole "we have update_data structure, in which
recursive_prefix, sm_path and prefix members in it; please set the
displaypath member based on these values" should become a helper
function, e.g.

	static const char *displaypath_from_update_data(struct update_data *u)
	{
		char *pp, *ret;

		if (u->recursive_prefix)
			pp = xstrfmt("%s%s", u->recursive_prefix, u->sm_path);
		else
			pp = xstrdup(u->sm_path);

		ret = get_submodule_displaypath(pp, u->prefix);
		free(pp);
		return ret;
	}

to avoid duplicated computation.

But the whole thing may become moot, as there seems to be a move to
get rid of submodule--helper.c altogether?

I'll refrain from touching this patch and instead redirect it to
Glen; perhaps removal of submodule--helper.c involves moving the
code here to another file or something, in which case it is far
easier if I outsource that to somebody who is actually working on
the file ;-)

Thanks.

>  		next.prefix = NULL;
>  		oidcpy(&next.oid, null_oid());
>  		oidcpy(&next.suboid, null_oid());

Re: [PATCH 03/11] submodule--helper: avoid memory leak in `update_submodule()`

[Glen Choo]

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> Reported by Coverity.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>>  builtin/submodule--helper.c | 2 ++
>>  1 file changed, 2 insertions(+)
>
>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 5c77dfcffee..d7b8004b933 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data)
>>  
>>  		next.recursive_prefix = get_submodule_displaypath(prefixed_path,
>>  								  update_data->prefix);
>> +		free(prefixed_path);
>> +
>
> This function has two very similar code block that computes
> prefixed_path depending on the same condition, and one frees the
> variable correctly while the other one (i.e. this one) forgets to do
> so, which is irritating to see.

Good eye, I hadn't noticed this.

In this case, we're computing the same thing twice in the function -
first unconditionally, then conditionally. So we can actually just reuse
the first result by doing something like

		next.recursive_prefix = xstrfmt("%s/", update_data->displaypath);

(note that we need to append a "/" at the end)

> Perhaps the whole "we have update_data structure, in which
> recursive_prefix, sm_path and prefix members in it; please set the
> displaypath member based on these values" should become a helper
> function, e.g.
>
> 	static const char *displaypath_from_update_data(struct update_data *u)
> 	{
> 		char *pp, *ret;
>
> 		if (u->recursive_prefix)
> 			pp = xstrfmt("%s%s", u->recursive_prefix, u->sm_path);
> 		else
> 			pp = xstrdup(u->sm_path);
>
> 		ret = get_submodule_displaypath(pp, u->prefix);
> 		free(pp);
> 		return ret;
> 	}
>
> to avoid duplicated computation.

Even better, I recently noticed that .recursive_prefix can probably just
be replaced with "--super-prefix", and that will let us dispatch to
get_submodule_displaypath() without this extra dance here.

I'll flesh that out into a series and send it soon.

> But the whole thing may become moot, as there seems to be a move to
> get rid of submodule--helper.c altogether?
>
>
> I'll refrain from touching this patch and instead redirect it to
> Glen; perhaps removal of submodule--helper.c involves moving the
> code here to another file or something, in which case it is far
> easier if I outsource that to somebody who is actually working on
> the file ;-)

cc-ed Ævar, who knows even more than I do :)

git-submodule.sh is definitely going away soon \o/ but I don't think we
have plans to get rid of submodule--helper.c just yet, so I think we
should probably still keep this patch.

>
> Thanks.
>
>>  		next.prefix = NULL;
>>  		oidcpy(&next.oid, null_oid());
>>  		oidcpy(&next.suboid, null_oid());

[PATCH 04/11] get_oid_with_context_1(): avoid use-after-free

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 561287d342cc (object-name: reject trees found in the index,
2022-04-26), we changed the code to release the memory stored under the
`new_path` variable a bit earlier.

However, at that point the variable `cp` still points to `new_path` (if
non-`NULL`), and we _then_ pass that pointer to
`reject_tree_in_index()`.

Let's make sure that the pointer still points to allocated memory by
moving the original `free()` call back where it was, and adding another
one in the early return code path.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 object-name.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/object-name.c b/object-name.c
index 4d2746574cd..228c12a554c 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1971,12 +1971,14 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
 			    memcmp(ce->name, cp, namelen))
 				break;
 			if (ce_stage(ce) == stage) {
-				free(new_path);
 				if (reject_tree_in_index(repo, only_to_die, ce,
-							 stage, prefix, cp))
+							 stage, prefix, cp)) {
+					free(new_path);
 					return -1;
+				}
 				oidcpy(oid, &ce->oid);
 				oc->mode = ce->ce_mode;
+				free(new_path);
 				return 0;
 			}
 			pos++;
-- 
gitgitgadget

Re: [PATCH 04/11] get_oid_with_context_1(): avoid use-after-free

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

>  			if (ce_stage(ce) == stage) {
> -				free(new_path);
>  				if (reject_tree_in_index(repo, only_to_die, ce,
> -							 stage, prefix, cp))
> +							 stage, prefix, cp)) {
> +					free(new_path);
>  					return -1;
> +				}
>  				oidcpy(oid, &ce->oid);
>  				oc->mode = ce->ce_mode;
> +				free(new_path);
>  				return 0;
>  			}

Hmph.  Without the "lets make sure we do not leak in the error code
path", it would have been no brainer to avoid this bug in the
original version.  Of course the postimage of the above hunk is
correct, but with extra free() sprinkled, it became ugly to the eye.

I wonder if the following is easier to follow.

Thanks.

 object-name.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git c/object-name.c w/object-name.c
index 4d2746574c..57188db7b0 100644
--- c/object-name.c
+++ w/object-name.c
@@ -1971,13 +1971,16 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
 			    memcmp(ce->name, cp, namelen))
 				break;
 			if (ce_stage(ce) == stage) {
+				int ret = -1;
+
+				if (!reject_tree_in_index(repo, only_to_die, ce,
+							  stage, prefix, cp)) {
+					oidcpy(oid, &ce->oid);
+					oc->mode = ce->ce_mode;
+					ret = 0;
+				}
 				free(new_path);
-				if (reject_tree_in_index(repo, only_to_die, ce,
-							 stage, prefix, cp))
-					return -1;
-				oidcpy(oid, &ce->oid);
-				oc->mode = ce->ce_mode;
-				return 0;
+				return ret;
 			}
 			pos++;
 		}

[PATCH 05/11] submodule-config: avoid memory leak

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 961b130d20c9 (branch: add --recurse-submodules option for branch
creation, 2022-01-28), a funny pattern was introduced where first some
struct is `xmalloc()`ed, then we resize an array whose element type is
the same struct, and then the first struct's contents are copied into
the last element of that array.

Crucially, the `xmalloc()`ed memory never gets released.

Let's avoid that memory leak and that memory allocation dance altogether
by first reallocating the array, then using a pointer to the last array
element to go forward.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 submodule-config.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index ce3beaf5d4f..51ecbe901ec 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -756,7 +756,10 @@ static void traverse_tree_submodules(struct repository *r,
 
 		if (S_ISGITLINK(name_entry->mode) &&
 		    is_tree_submodule_active(r, root_tree, tree_path)) {
-			st_entry = xmalloc(sizeof(*st_entry));
+			ALLOC_GROW(out->entries, out->entry_nr + 1,
+				   out->entry_alloc);
+			st_entry = &out->entries[out->entry_nr++];
+
 			st_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry));
 			*st_entry->name_entry = *name_entry;
 			st_entry->submodule =
@@ -766,9 +769,6 @@ static void traverse_tree_submodules(struct repository *r,
 						root_tree))
 				FREE_AND_NULL(st_entry->repo);
 
-			ALLOC_GROW(out->entries, out->entry_nr + 1,
-				   out->entry_alloc);
-			out->entries[out->entry_nr++] = *st_entry;
 		} else if (S_ISDIR(name_entry->mode))
 			traverse_tree_submodules(r, root_tree, tree_path,
 						 &name_entry->oid, out);
-- 
gitgitgadget

Re: [PATCH 05/11] submodule-config: avoid memory leak

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 961b130d20c9 (branch: add --recurse-submodules option for branch
> creation, 2022-01-28), a funny pattern was introduced where first some
> struct is `xmalloc()`ed, then we resize an array whose element type is
> the same struct, and then the first struct's contents are copied into
> the last element of that array.

Sigh.  The original is butt ugly, with this strange pattern and
structure assignments etc.  I wonder how something like this slipped
through our reviews.

I wonder if it would help for me to stop trusting reviews by less
experienced reviewers too much, and instead give sanity checks to
more patches myself from now on, but I certainly cannot afford the
time and my mental health to do so for all the patches X-<.

Will queue.

>  		if (S_ISGITLINK(name_entry->mode) &&
>  		    is_tree_submodule_active(r, root_tree, tree_path)) {
> -			st_entry = xmalloc(sizeof(*st_entry));
> +			ALLOC_GROW(out->entries, out->entry_nr + 1,
> +				   out->entry_alloc);
> +			st_entry = &out->entries[out->entry_nr++];
> +
>  			st_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry));
>  			*st_entry->name_entry = *name_entry;
>  			st_entry->submodule =
> @@ -766,9 +769,6 @@ static void traverse_tree_submodules(struct repository *r,
>  						root_tree))
>  				FREE_AND_NULL(st_entry->repo);
>  
> -			ALLOC_GROW(out->entries, out->entry_nr + 1,
> -				   out->entry_alloc);
> -			out->entries[out->entry_nr++] = *st_entry;
>  		} else if (S_ISDIR(name_entry->mode))
>  			traverse_tree_submodules(r, root_tree, tree_path,
>  						 &name_entry->oid, out);

Re: [PATCH 05/11] submodule-config: avoid memory leak

[Glen Choo]

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

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> In 961b130d20c9 (branch: add --recurse-submodules option for branch
>> creation, 2022-01-28), a funny pattern was introduced where first some
>> struct is `xmalloc()`ed, then we resize an array whose element type is
>> the same struct, and then the first struct's contents are copied into
>> the last element of that array.
>
> Sigh.  The original is butt ugly, with this strange pattern and
> structure assignments etc.  I wonder how something like this slipped
> through our reviews.

Gah, I have no excuses for bad code I've already written, I can only
strive to write better code next time.

Thanks Johannes for spotting and fixing this.

> I wonder if it would help for me to stop trusting reviews by less
> experienced reviewers too much, and instead give sanity checks to
> more patches myself from now on, but I certainly cannot afford the
> time and my mental health to do so for all the patches X-<.

I seem to recall that this was reviewed by fairly experienced folks. I'm
guessing it slipped through due to reviewer fatigue, which might be a
good argument for adding more tooling to lighten reviewer load/patch up
the gaps.

>
> Will queue.
>
>>  		if (S_ISGITLINK(name_entry->mode) &&
>>  		    is_tree_submodule_active(r, root_tree, tree_path)) {
>> -			st_entry = xmalloc(sizeof(*st_entry));
>> +			ALLOC_GROW(out->entries, out->entry_nr + 1,
>> +				   out->entry_alloc);
>> +			st_entry = &out->entries[out->entry_nr++];
>> +
>>  			st_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry));
>>  			*st_entry->name_entry = *name_entry;
>>  			st_entry->submodule =
>> @@ -766,9 +769,6 @@ static void traverse_tree_submodules(struct repository *r,
>>  						root_tree))
>>  				FREE_AND_NULL(st_entry->repo);
>>  
>> -			ALLOC_GROW(out->entries, out->entry_nr + 1,
>> -				   out->entry_alloc);
>> -			out->entries[out->entry_nr++] = *st_entry;
>>  		} else if (S_ISDIR(name_entry->mode))
>>  			traverse_tree_submodules(r, root_tree, tree_path,
>>  						 &name_entry->oid, out);

[PATCH 06/11] pack-redundant: avoid using uninitialized memory

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In the `add_pack()` function, we forgot to initialize the field `next`,
which could potentially lead to readin uninitialized memory later.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pack-redundant.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index ed9b9013a5f..1f7da1f68b6 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -526,6 +526,7 @@ static struct pack_list * add_pack(struct packed_git *p)
 	}
 	l.all_objects_size = l.remaining_objects->size;
 	l.unique_objects = NULL;
+	l.next = NULL;
 	if (p->pack_local)
 		return pack_list_insert(&local_packs, &l);
 	else
-- 
gitgitgadget

Re: [PATCH 06/11] pack-redundant: avoid using uninitialized memory

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In the `add_pack()` function, we forgot to initialize the field `next`,
> which could potentially lead to readin uninitialized memory later.
>
> Reported by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/pack-redundant.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
> index ed9b9013a5f..1f7da1f68b6 100644
> --- a/builtin/pack-redundant.c
> +++ b/builtin/pack-redundant.c
> @@ -526,6 +526,7 @@ static struct pack_list * add_pack(struct packed_git *p)
>  	}
>  	l.all_objects_size = l.remaining_objects->size;
>  	l.unique_objects = NULL;
> +	l.next = NULL;
>  	if (p->pack_local)
>  		return pack_list_insert(&local_packs, &l);
>  	else
		return pack_list_insert(&altodb_packs, &l);


The pack_list_insert reads like so:

static inline struct pack_list * pack_list_insert(struct pack_list **pl,
					   struct pack_list *entry)
{
	struct pack_list *p = xmalloc(sizeof(struct pack_list));
	memcpy(p, entry, sizeof(struct pack_list));
	p->next = *pl;
	*pl = p;
	return p;
}


IOW, we prepare members of "l", pass it as "entry" to pack_list_insert(),
but the helper function allocates a new piece of memory, copies "l"
to it, and then sets the .next member of it to a reasonable value.

And after that, the "l" that was merely used as a template goes out
of scope.

So I think this is a false positive.  memcpy(p) does copy the
uninitialized garbage that is held in entry->next to p->next, but it
is overwritten with a reasonable address immediately after that.

[PATCH 07/11] submodule--helper: avoid memory leak when fetching submodules

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In c51f8f94e5b3 (submodule--helper: run update procedures from C,
2021-08-24), we added code that first obtains the default remote, and
then adds that to a `strvec`.

However, we never released the default remote's memory.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/submodule--helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d7b8004b933..9a8248ffe6a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2208,6 +2208,7 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str
 		char *hex = oid_to_hex(oid);
 		char *remote = get_default_remote();
 		strvec_pushl(&cp.args, remote, hex, NULL);
+		free(remote);
 	}
 
 	return run_command(&cp);
-- 
gitgitgadget

Re: [PATCH 07/11] submodule--helper: avoid memory leak when fetching submodules

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In c51f8f94e5b3 (submodule--helper: run update procedures from C,
> 2021-08-24), we added code that first obtains the default remote, and
> then adds that to a `strvec`.
>
> However, we never released the default remote's memory.
>
> Reported by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/submodule--helper.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d7b8004b933..9a8248ffe6a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2208,6 +2208,7 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str
>  		char *hex = oid_to_hex(oid);
>  		char *remote = get_default_remote();
>  		strvec_pushl(&cp.args, remote, hex, NULL);
> +		free(remote);

Trivially correct.  Will queue.

>  	}
>  
>  	return run_command(&cp);

[PATCH 08/11] read_index_from(): avoid memory leak

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 998330ac2e7c (read-cache: look for shared index files next to the
index, too, 2021-08-26), we added code that allocates memory to store
the base path of a shared index, but we never released that memory.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 read-cache.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index e61af3a3d4d..76f372ff917 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2473,15 +2473,15 @@ int read_index_from(struct index_state *istate, const char *path,
 				   the_repository, "%s", base_path);
 	if (!ret) {
 		char *path_copy = xstrdup(path);
-		const char *base_path2 = xstrfmt("%s/sharedindex.%s",
-						 dirname(path_copy),
-						 base_oid_hex);
+		char *base_path2 = xstrfmt("%s/sharedindex.%s",
+					   dirname(path_copy), base_oid_hex);
 		free(path_copy);
 		trace2_region_enter_printf("index", "shared/do_read_index",
 					   the_repository, "%s", base_path2);
 		ret = do_read_index(split_index->base, base_path2, 1);
 		trace2_region_leave_printf("index", "shared/do_read_index",
 					   the_repository, "%s", base_path2);
+		free(base_path2);
 	}
 	if (!oideq(&split_index->base_oid, &split_index->base->oid))
 		die(_("broken index, expect %s in %s, got %s"),
-- 
gitgitgadget

[PATCH 08/11] read_index_from(): avoid memory leak

[Tom Levy]

Hi,

I think there is another problem with this code: later on it refers to
the 'base_path' variable, but I think it should refer to 'base_path2'
if the first path did not exist.

How about the following patch instead (or in addition)?

Regards,
Tom Levy


-- >8 --

Subject: [PATCH] read_index_from(): use second path consistently, avoid memory
 leak

In 998330ac2e7c (read-cache: look for shared index files next to the
index, too, 2021-08-26), we added code that allocates memory to store
the path of a second shared base index, but we never released that
memory. Also, later code (e.g. the call to freshen_shared_index())
continued to use the first path even when the second path was
selected.

Modify the code to store the second path in the original variable
(after freeing the first path), so that the later code will use the
second path (and also free it).

The memory leak was reported by Coverity.

Co-authored-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Tom Levy <tomlevy93@gmail.com>
---
 read-cache.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 4df97e185e..48f2b9aa9d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2459,15 +2459,15 @@ int read_index_from(struct index_state *istate, const char *path,
                                   the_repository, "%s", base_path);
        if (!ret) {
                char *path_copy = xstrdup(path);
-               const char *base_path2 = xstrfmt("%s/sharedindex.%s",
-                                                dirname(path_copy),
-                                                base_oid_hex);
+               free(base_path);
+               base_path = xstrfmt("%s/sharedindex.%s", dirname(path_copy),
+                                   base_oid_hex);
                free(path_copy);
                trace2_region_enter_printf("index", "shared/do_read_index",
-                                          the_repository, "%s", base_path2);
-               ret = do_read_index(split_index->base, base_path2, 1);
+                                          the_repository, "%s", base_path);
+               ret = do_read_index(split_index->base, base_path, 1);
                trace2_region_leave_printf("index", "shared/do_read_index",
-                                          the_repository, "%s", base_path2);
+                                          the_repository, "%s", base_path);
        }
        if (!oideq(&split_index->base_oid, &split_index->base->oid))
                die(_("broken index, expect %s in %s, got %s"),
--
2.30.2

[PATCH 09/11] pack-mtimes: avoid closing a bogus file descriptor

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 94cd775a6c52 (pack-mtimes: support reading .mtimes files,
2022-05-20), code was added to close the file descriptor corresponding
to the mtimes file.

However, it is possible that opening that file failed, in which case we
are closing a file descriptor with the value `-1`. Let's guard that
`close()` call.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 pack-mtimes.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/pack-mtimes.c b/pack-mtimes.c
index 0e0aafdcb06..0f9785fc5e4 100644
--- a/pack-mtimes.c
+++ b/pack-mtimes.c
@@ -89,7 +89,8 @@ cleanup:
 		*data_p = data;
 	}
 
-	close(fd);
+	if (fd >= 0)
+		close(fd);
 	return ret;
 }
 
-- 
gitgitgadget

Re: [PATCH 09/11] pack-mtimes: avoid closing a bogus file descriptor

[Taylor Blau]

On Wed, Jun 15, 2022 at 11:35:43PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 94cd775a6c52 (pack-mtimes: support reading .mtimes files,
> 2022-05-20), code was added to close the file descriptor corresponding
> to the mtimes file.
>
> However, it is possible that opening that file failed, in which case we
> are closing a file descriptor with the value `-1`. Let's guard that
> `close()` call.

Nice catch. I agree with your assessment and fix. Thanks for spotting
and fixing!

Thanks,
Taylor

[PATCH 10/11] relative_url(): fix incorrect condition

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In 63e95beb085c (submodule: port resolve_relative_url from shell to C,
2016-04-15), we added a loop over `url` where we are looking for `../`
or `./` components.

The loop condition we used is the pointer `url` itself, which is clearly
not what we wanted.

Pointed out by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 9b9bbfe80ec..bded6acbfe8 100644
--- a/remote.c
+++ b/remote.c
@@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url,
 	 * When the url starts with '../', remove that and the
 	 * last directory in remoteurl.
 	 */
-	while (url) {
+	while (*url) {
 		if (starts_with_dot_dot_slash_native(url)) {
 			url += 3;
 			colonsep |= chop_last_dir(&remoteurl, is_relative);
-- 
gitgitgadget

Re: [PATCH 10/11] relative_url(): fix incorrect condition

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 63e95beb085c (submodule: port resolve_relative_url from shell to C,
> 2016-04-15), we added a loop over `url` where we are looking for `../`
> or `./` components.
>
> The loop condition we used is the pointer `url` itself, which is clearly
> not what we wanted.

8/11, 9/11, and 10/11 are trivially correct.  Will queue.

Thanks.


>
> Pointed out by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/remote.c b/remote.c
> index 9b9bbfe80ec..bded6acbfe8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url,
>  	 * When the url starts with '../', remove that and the
>  	 * last directory in remoteurl.
>  	 */
> -	while (url) {
> +	while (*url) {
>  		if (starts_with_dot_dot_slash_native(url)) {
>  			url += 3;
>  			colonsep |= chop_last_dir(&remoteurl, is_relative);

Re: [PATCH 10/11] relative_url(): fix incorrect condition

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 15 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 63e95beb085c (submodule: port resolve_relative_url from shell to C,
> 2016-04-15), we added a loop over `url` where we are looking for `../`
> or `./` components.
>
> The loop condition we used is the pointer `url` itself, which is clearly
> not what we wanted.

Clearly, but having looked at this I think it's useful to note that
coverity must be (I don't know what it actually emitted) be complaining
about the "url" condition being useless, i.e. it's the same as a "while
(1)", not that we run off the end of the string.

I.e. due to the way those start_with*() functions work we would abort
early if we were running off th end of the string.

> Pointed out by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/remote.c b/remote.c
> index 9b9bbfe80ec..bded6acbfe8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url,
>  	 * When the url starts with '../', remove that and the
>  	 * last directory in remoteurl.
>  	 */
> -	while (url) {
> +	while (*url) {
>  		if (starts_with_dot_dot_slash_native(url)) {
>  			url += 3;
>  			colonsep |= chop_last_dir(&remoteurl, is_relative);

Which I tested with this:
	
	diff --git a/remote.c b/remote.c
	index 9b9bbfe80ec..e049bbb791c 100644
	--- a/remote.c
	+++ b/remote.c
	@@ -2846,14 +2846,17 @@ char *relative_url(const char *remote_url, const char *url,
	 	 * When the url starts with '../', remove that and the
	 	 * last directory in remoteurl.
	 	 */
	-	while (url) {
	+	while (1) {
	 		if (starts_with_dot_dot_slash_native(url)) {
	 			url += 3;
	 			colonsep |= chop_last_dir(&remoteurl, is_relative);
	-		} else if (starts_with_dot_slash_native(url))
	+		} else if (starts_with_dot_slash_native(url)) {
	 			url += 2;
	-		else
	-			break;
	+		} else if (!*url) {
	+			BUG("ran off the end of our url?");
	+		} else {
	+			break; 
	+		}
	 	}
	 	strbuf_reset(&sb);
	 	strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
	
Which will fail e.g. on this test in t3420-rebase-autostash.sh:

	+ git submodule add ./ sub
	BUG: remote.c:2856: ran off the end of our url?
	Aborted

I worried a bit about this since we released this with v2.9.0, so in all
this time we haven't been infinitely looping on this case, or at least
haven't had any reports about that.

So if we hadn't been catching this in starts_with_*() I wouldn't be
confident that this was the correct fix, yes it's almost certainly not
what not what was intended, but if we change it to that are we confident
that the side-effects are going to be what we want?

But in this case I'm pretty sure that the behavior before/after will be
the same, and that the only change will be to make coverity happier, and
the code less confusing.

Re: [PATCH 10/11] relative_url(): fix incorrect condition

[Junio C Hamano]

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

>> -	while (url) {
>> +	while (*url) {
>>  		if (starts_with_dot_dot_slash_native(url)) {
>>  			url += 3;
>>  			colonsep |= chop_last_dir(&remoteurl, is_relative);
>
> Which I tested with this:
> 	
> 	diff --git a/remote.c b/remote.c
> 	index 9b9bbfe80ec..e049bbb791c 100644
> 	--- a/remote.c
> 	+++ b/remote.c
> 	@@ -2846,14 +2846,17 @@ char *relative_url(const char *remote_url, const char *url,
> 	 	 * When the url starts with '../', remove that and the
> 	 	 * last directory in remoteurl.
> 	 	 */
> 	-	while (url) {
> 	+	while (1) {
> 	 		if (starts_with_dot_dot_slash_native(url)) {
> 	 			url += 3;
> 	 			colonsep |= chop_last_dir(&remoteurl, is_relative);
> 	-		} else if (starts_with_dot_slash_native(url))
> 	+		} else if (starts_with_dot_slash_native(url)) {
> 	 			url += 2;
> 	-		else
> 	-			break;
> 	+		} else if (!*url) {
> 	+			BUG("ran off the end of our url?");
> 	+		} else {
> 	+			break; 
> 	+		}
> 	 	}
> 	 	strbuf_reset(&sb);
> 	 	strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
> 	
> Which will fail e.g. on this test in t3420-rebase-autostash.sh:
>
> 	+ git submodule add ./ sub
> 	BUG: remote.c:2856: ran off the end of our url?
> 	Aborted

Sorry, but I am confused.  I do not see the point of that test
before the BUG.

I agree that this loop over url is "we want to loop forever, and
stop immediately when url does not start with ../ or ./" infinite
loop, and it is better written with "while(1)".  If you do not make
any other changes that confuses me, we'll get this:

-	while (*url) {
+	while (1) {
		if (starts_with_dot_dot_slash_native(url)) {
			url += 3;
			colonsep |= chop_last_dir(&remoteurl, is_relative);
		} else if (starts_with_dot_slash_native(url))
			url += 2;
		else
			break;
	}

We know url upon entry is not NULL.  starts_with_dot_*() ends up
calling dir.c:path_match_flags() and the first thing it does is to
return if the byte is not '.'.  So the difference is whether you
leave the loop with the "while" condition, or you fail the two
starts_with_dot_*() calls and hit the "break" at the end in the
loop.

IOW, running of the end of the URL is not a BUG, is it?

Re: [PATCH 10/11] relative_url(): fix incorrect condition

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In 63e95beb085c (submodule: port resolve_relative_url from shell to C,
> 2016-04-15), we added a loop over `url` where we are looking for `../`
> or `./` components.
>
> The loop condition we used is the pointer `url` itself, which is clearly
> not what we wanted.
>
> Pointed out by Coverity.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/remote.c b/remote.c
> index 9b9bbfe80ec..bded6acbfe8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url,
>  	 * When the url starts with '../', remove that and the
>  	 * last directory in remoteurl.
>  	 */
> -	while (url) {
> +	while (*url) {
>  		if (starts_with_dot_dot_slash_native(url)) {
>  			url += 3;
>  			colonsep |= chop_last_dir(&remoteurl, is_relative);
		} else if (starts_with_dot_slash_native(url))
			url += 2;
		else
			break;
	}

Looking at the "problematic" code again, the original is bad in the
sense that it is utterly misleading, but not quite "incorrect", I
would think.

In other words, what the original wanted to write is an infinite
loop that uses an explicit "break" to terminate the iteration, so we
should have written "while (1)" in the first place.

We know that earlier part of the function already depends on url
being not NULL, and nothing in the loop makes it NULL.  *url that is
NUL would make the two starts_with_frotz() tests fail and causes the
loop to terminate by hitting "break", so while it is correct to
check *url, it is wasting cycles.

But this patch is not wrong per-se, so I still have it in my tree
;-)

Thanks.

[PATCH 11/11] bug_fl(): add missing `va_end()` call

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

According to the manual:

	Each invocation of va_copy() must be matched by a corresponding
	invocation of va_end() in the  same function.

Note: There is another instance of `va_copy()` in `usage.c` that is
missing a `va_end()` call, in `BUG_vfl()`. It does not matter there,
though, because that function either `exit()`s or `abort()`s, anyway.

Reported by Coverity.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 usage.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/usage.c b/usage.c
index 79900d0287f..4849eb75785 100644
--- a/usage.c
+++ b/usage.c
@@ -343,6 +343,7 @@ void bug_fl(const char *file, int line, const char *fmt, ...)
 	BUG_vfl_common(file, line, fmt, ap);
 	va_end(ap);
 	trace2_cmd_error_va(fmt, cp);
+	va_end(cp);
 }
 
 #ifdef SUPPRESS_ANNOTATED_LEAKS
-- 
gitgitgadget

Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call

[Jeff King]

On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> According to the manual:
> 
> 	Each invocation of va_copy() must be matched by a corresponding
> 	invocation of va_end() in the  same function.
> 
> Note: There is another instance of `va_copy()` in `usage.c` that is
> missing a `va_end()` call, in `BUG_vfl()`. It does not matter there,
> though, because that function either `exit()`s or `abort()`s, anyway.
> 
> Reported by Coverity.

This was introduced by the recent 0cc05b044f (usage.c: add a non-fatal
bug() function to go with BUG(), 2022-06-02). But there's a much worse
bug in the same function. The code introduced by that patch does:

  va_list ap, cp;
  [...]
  va_copy(cp, ap);
  va_start(ap, fmt);

So "cp" is copied from "ap" before we have actually initialized "ap".
It's surprising that this works at all. The two lines should be flipped.

IMHO, since we're in the actual varargs function itself, it would be
simpler to just bracket each use with start/end, rather than copying,
like:

diff --git a/usage.c b/usage.c
index 79900d0287..56e29d6cd6 100644
--- a/usage.c
+++ b/usage.c
@@ -334,15 +334,17 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
 int bug_called_must_BUG;
 void bug_fl(const char *file, int line, const char *fmt, ...)
 {
-	va_list ap, cp;
+	va_list ap;
 
 	bug_called_must_BUG = 1;
 
-	va_copy(cp, ap);
 	va_start(ap, fmt);
 	BUG_vfl_common(file, line, fmt, ap);
 	va_end(ap);
-	trace2_cmd_error_va(fmt, cp);
+
+	va_start(ap, fmt);
+	trace2_cmd_error_va(fmt, ap);
+	va_end(ap);
 }
 
 #ifdef SUPPRESS_ANNOTATED_LEAKS

but I am happy with any solution that is correct. :)

-Peff

Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> 
>> According to the manual:
>> 
>> 	Each invocation of va_copy() must be matched by a corresponding
>> 	invocation of va_end() in the  same function.
>> 
>> Note: There is another instance of `va_copy()` in `usage.c` that is
>> missing a `va_end()` call, in `BUG_vfl()`. It does not matter there,
>> though, because that function either `exit()`s or `abort()`s, anyway.
>> 
>> Reported by Coverity.
>
> This was introduced by the recent 0cc05b044f (usage.c: add a non-fatal
> bug() function to go with BUG(), 2022-06-02). But there's a much worse
> bug in the same function. The code introduced by that patch does:
>
>   va_list ap, cp;
>   [...]
>   va_copy(cp, ap);
>   va_start(ap, fmt);
>
> So "cp" is copied from "ap" before we have actually initialized "ap".
> It's surprising that this works at all. The two lines should be flipped.

Yes, it is surprising.  Perhaps it is not working at all.

Thanks for an extra set of eyeballs.

> IMHO, since we're in the actual varargs function itself, it would be
> simpler to just bracket each use with start/end, rather than copying,
> like:
>
> diff --git a/usage.c b/usage.c
> index 79900d0287..56e29d6cd6 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -334,15 +334,17 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
>  int bug_called_must_BUG;
>  void bug_fl(const char *file, int line, const char *fmt, ...)
>  {
> -	va_list ap, cp;
> +	va_list ap;
>  
>  	bug_called_must_BUG = 1;
>  
> -	va_copy(cp, ap);
>  	va_start(ap, fmt);
>  	BUG_vfl_common(file, line, fmt, ap);
>  	va_end(ap);
> -	trace2_cmd_error_va(fmt, cp);
> +
> +	va_start(ap, fmt);
> +	trace2_cmd_error_va(fmt, ap);
> +	va_end(ap);
>  }
>  
>  #ifdef SUPPRESS_ANNOTATED_LEAKS
>
> but I am happy with any solution that is correct. :)
>
> -Peff

Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 15 2022, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
>
>> On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote:
>>
>>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>> 
>>> According to the manual:
>>> 
>>> 	Each invocation of va_copy() must be matched by a corresponding
>>> 	invocation of va_end() in the  same function.
>>> 
>>> Note: There is another instance of `va_copy()` in `usage.c` that is
>>> missing a `va_end()` call, in `BUG_vfl()`. It does not matter there,
>>> though, because that function either `exit()`s or `abort()`s, anyway.
>>> 
>>> Reported by Coverity.
>>
>> This was introduced by the recent 0cc05b044f (usage.c: add a non-fatal
>> bug() function to go with BUG(), 2022-06-02). But there's a much worse
>> bug in the same function. The code introduced by that patch does:
>>
>>   va_list ap, cp;
>>   [...]
>>   va_copy(cp, ap);
>>   va_start(ap, fmt);
>>
>> So "cp" is copied from "ap" before we have actually initialized "ap".
>> It's surprising that this works at all. The two lines should be flipped.
>
> Yes, it is surprising.  Perhaps it is not working at all.
>
> Thanks for an extra set of eyeballs.

It's "working" now in the sense that we run this code, and even test
that trace2 output specifically, see the tests in 0cc05b044fd (usage.c:
add a non-fatal bug() function to go with BUG(), 2022-06-02).

But obviously that's a bad use of the varargs API, I just don't know how
we've been getting away with it in practice, sorry about that.

The fix Peff's got here LGTM. I can (re)submit it with
format-patch+send-email after giving it a commit message describing the
issue if you'd like, but the change would be the same.

I'm also happy for you to pick up the upthread directly, but I don't see
that you did that already in one of the integration branches (but
perhaps I missed one).

Just let me know, but I didn't want to re-submit this without asking, in
case I'd step on any toes (or submit something in duplicate).

Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call

[Junio C Hamano]

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

> But obviously that's a bad use of the varargs API, I just don't know how
> we've been getting away with it in practice, sorry about that.

Exactly.  We three all expressed our surprises on why it "works".
Nobody offered an explanation, though, which leaves us in suspense
;-)

> The fix Peff's got here LGTM. I can (re)submit it with
> format-patch+send-email after giving it a commit message describing the
> issue if you'd like, but the change would be the same.

Yup, I think the code change there looks the most sensible.

Thanks, all.

Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call

[Jeff King]

On Thu, Jun 16, 2022 at 11:03:25AM -0700, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > But obviously that's a bad use of the varargs API, I just don't know how
> > we've been getting away with it in practice, sorry about that.
> 
> Exactly.  We three all expressed our surprises on why it "works".
> Nobody offered an explanation, though, which leaves us in suspense
> ;-)

Being the curious sort, I ran it in a debugger. And indeed, "cp" is
filled with uninitialized garbage. The reason it works is that the test
calls bug() with a format string that does not contain any placeholders.
And thus our eventual call to vsnprintf() does not ever look at "cp" at
all!

> > The fix Peff's got here LGTM. I can (re)submit it with
> > format-patch+send-email after giving it a commit message describing the
> > issue if you'd like, but the change would be the same.
> 
> Yup, I think the code change there looks the most sensible.

I'll wrap it up with a commit message and modify the test to be more
thorough.

-Peff

[PATCH] bug_fl(): correctly initialize trace2 va_list

[Jeff King]

On Thu, Jun 16, 2022 at 03:20:08PM -0400, Jeff King wrote:

> > > The fix Peff's got here LGTM. I can (re)submit it with
> > > format-patch+send-email after giving it a commit message describing the
> > > issue if you'd like, but the change would be the same.
> > 
> > Yup, I think the code change there looks the most sensible.
> 
> I'll wrap it up with a commit message and modify the test to be more
> thorough.

Here it is. This can replace Johannes's patch 11.

-- >8 --
Subject: bug_fl(): correctly initialize trace2 va_list

The code added 0cc05b044f (usage.c: add a non-fatal bug() function to go
with BUG(), 2022-06-02) sets up two va_list variables: one to output to
stderr, and one to trace2. But the order of initialization is wrong:

  va_list ap, cp;
  va_copy(cp, ap);
  va_start(ap, fmt);

We copy the contents of "ap" into "cp" before it is initialized, meaning
it is full of garbage. The two should be swapped.

However, there's another bug, noticed by Johannes Schindelin: we forget
to call va_end() for the copy. So instead of just fixing the copy's
initialization, let's do two separate start/end pairs. This is allowed
by the standard, and we don't need to use copy here since we have access
to the original varargs. Matching the pairs with the calls makes it more
obvious that everything is being done correctly.

Note that we do call bug_fl() in the tests, but it didn't trigger this
problem because our format string doesn't have any placeholders. So even
though we were passing a garbage va_list through the stack, nobody ever
needed to look at it. We can easily adjust one of the trace2 tests to
trigger this, both for bug() and for BUG(). The latter isn't broken, but
it's nice to exercise both a bit more. Without the fix in this patch
(but with the test change), the bug() case causes a segfault.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/helper/test-trace2.c | 4 ++--
 usage.c                | 8 +++++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
index 180c7f53f3..a714130ece 100644
--- a/t/helper/test-trace2.c
+++ b/t/helper/test-trace2.c
@@ -224,8 +224,8 @@ static int ut_009bug_BUG(int argc, const char **argv)
 
 static int ut_010bug_BUG(int argc, const char **argv)
 {
-	bug("a bug message");
-	BUG("a BUG message");
+	bug("a %s message", "bug");
+	BUG("a %s message", "BUG");
 }
 
 /*
diff --git a/usage.c b/usage.c
index 79900d0287..56e29d6cd6 100644
--- a/usage.c
+++ b/usage.c
@@ -334,15 +334,17 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)
 int bug_called_must_BUG;
 void bug_fl(const char *file, int line, const char *fmt, ...)
 {
-	va_list ap, cp;
+	va_list ap;
 
 	bug_called_must_BUG = 1;
 
-	va_copy(cp, ap);
 	va_start(ap, fmt);
 	BUG_vfl_common(file, line, fmt, ap);
 	va_end(ap);
-	trace2_cmd_error_va(fmt, cp);
+
+	va_start(ap, fmt);
+	trace2_cmd_error_va(fmt, ap);
+	va_end(ap);
 }
 
 #ifdef SUPPRESS_ANNOTATED_LEAKS
-- 
2.37.0.rc0.352.g10876ef154

Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Being the curious sort, I ran it in a debugger. And indeed, "cp" is
> filled with uninitialized garbage. The reason it works is that the test
> calls bug() with a format string that does not contain any placeholders.
> And thus our eventual call to vsnprintf() does not ever look at "cp" at
> all!

;-)  Thanks.

Re: [PATCH 00/11] Coverity fixes

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> The release window is a fine time to concentrate on bug fixes,

Well, it certainly is better than random shiny new features or
refactoring for the sake of refactoring, but finding and fixing a
bug that has been with us since forever without anybody noticing is
not all that ultra-urgent, either ;-)

It is a good time to concentrate on finding and fixing regressions.

> and Coverity
> can help identify bugs. These are only a couple of fixes, Coverity reported
> many more, but most of the reports seemed either bogus or harmless (such as
> resource leaks in test helpers).

Thanks.

>
> Johannes Schindelin (11):
>   mingw: avoid accessing uninitialized memory in `is_executable()`
>   fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()`
>   submodule--helper: avoid memory leak in `update_submodule()`
>   get_oid_with_context_1(): avoid use-after-free
>   submodule-config: avoid memory leak
>   pack-redundant: avoid using uninitialized memory
>   submodule--helper: avoid memory leak when fetching submodules
>   read_index_from(): avoid memory leak
>   pack-mtimes: avoid closing a bogus file descriptor
>   relative_url(): fix incorrect condition
>   bug_fl(): add missing `va_end()` call
>
>  builtin/pack-redundant.c    | 1 +
>  builtin/submodule--helper.c | 3 +++
>  fsmonitor-settings.c        | 8 ++++++--
>  object-name.c               | 6 ++++--
>  pack-mtimes.c               | 3 ++-
>  read-cache.c                | 6 +++---
>  remote.c                    | 2 +-
>  run-command.c               | 2 +-
>  submodule-config.c          | 8 ++++----
>  usage.c                     | 1 +
>  10 files changed, 26 insertions(+), 14 deletions(-)
>
>
> base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1264%2Fdscho%2Fcoverity-fixes-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1264/dscho/coverity-fixes-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1264