Re: [PATCH] ref-filter: initialize empty name or email fields

Junio C Hamano [off-list ref] writes:

Mischa POSLAWSKY [off-list ref] writes:

quoted

Formatting $(taggername) on headerless tags such as v0.99 in Git
causes a SIGABRT with error "munmap_chunk(): invalid pointer",
because of an oversight in commit f0062d3b74 (ref-filter: free
item->value and item->value->s, 2018-10-19).

Signed-off-by: Mischa POSLAWSKY <redacted>
---
If I understand correctly, such tags cannot be produced normally anymore.
Therefore I'm unsure how to make tests, and if that is even warranted.

Thanks for spotting.

I am not sure if the approach taken by this patch is the right one,
though.  I didn't follow the call/dataflow thoroughly, but if we
replace unfree-able "" with NULL in these places, wouldn't
fill_missing_values() take care of them?

I think replacing these "" with NULL would be safe, but there are
many places that return xstrdup("") from inside the callees of
populate_value(), so the patch presented here would be more
consistent with the current practice, I think.

So let's take the patch as is, at least for now.  Thanks.

quoted

 ref-filter.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index f27cfc8c3e..7338cfc671 100644
--- a/ref-filter.c
+++ b/ref-filter.c

@@ -1028,7 +1028,7 @@ static const char *copy_name(const char *buf)
 		if (!strncmp(cp, " <", 2))
 			return xmemdupz(buf, cp - buf);
 	}
-	return "";
+	return xstrdup("");
 }
 
 static const char *copy_email(const char *buf)

@@ -1036,10 +1036,10 @@ static const char *copy_email(const char *buf)
 	const char *email = strchr(buf, '<');
 	const char *eoemail;
 	if (!email)
-		return "";
+		return xstrdup("");
 	eoemail = strchr(email, '>');
 	if (!eoemail)
-		return "";
+		return xstrdup("");
 	return xmemdupz(email, eoemail + 1 - email);
 }

Junio wrote:

Junio C Hamano [off-list ref] writes:

quoted

Mischa POSLAWSKY [off-list ref] writes:

quoted

Formatting $(taggername) on headerless tags such as v0.99 in Git
causes a SIGABRT with error "munmap_chunk(): invalid pointer",
because of an oversight in commit f0062d3b74 (ref-filter: free
item->value and item->value->s, 2018-10-19).

Signed-off-by: Mischa POSLAWSKY <redacted>
---
If I understand correctly, such tags cannot be produced normally anymore.
Therefore I'm unsure how to make tests, and if that is even warranted.

Thanks for spotting.

I am not sure if the approach taken by this patch is the right one,
though.  I didn't follow the call/dataflow thoroughly, but if we
replace unfree-able "" with NULL in these places, wouldn't
fill_missing_values() take care of them?

I think replacing these "" with NULL would be safe, but there are
many places that return xstrdup("") from inside the callees of
populate_value(), so the patch presented here would be more
consistent with the current practice, I think.

Indeed, I just copied the existing style.  Returning NULL seems to work,
but not something I'm confident to clean up here.

So let's take the patch as is, at least for now.  Thanks.

Thank you!

quoted

quoted

 ref-filter.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index f27cfc8c3e..7338cfc671 100644
--- a/ref-filter.c
+++ b/ref-filter.c

@@ -1028,7 +1028,7 @@ static const char *copy_name(const char *buf)
 		if (!strncmp(cp, " <", 2))
 			return xmemdupz(buf, cp - buf);
 	}
-	return "";
+	return xstrdup("");
 }
 
 static const char *copy_email(const char *buf)

@@ -1036,10 +1036,10 @@ static const char *copy_email(const char *buf)
 	const char *email = strchr(buf, '<');
 	const char *eoemail;
 	if (!email)
-		return "";
+		return xstrdup("");
 	eoemail = strchr(email, '>');
 	if (!eoemail)
-		return "";
+		return xstrdup("");
 	return xmemdupz(email, eoemail + 1 - email);
 }