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

Commit Message

Johannes Schindelin June 15, 2022, 11:35 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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 <[e-mail]>
---
 object-name.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Junio C Hamano June 16, 2022, 4:29 a.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
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

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++;