Re: [PATCH] mailmap: fix check for freeing memory

Stefan Beller [off-list ref] writes:

The condition as it is written in that line was most likely intended to
check for the pointer passed to free(), rather than checking for the
'repo_abbrev', which is already checked against being non null at the
beginning of the function.

[...]

-			if (repo_abbrev)
+			if (*repo_abbrev)
 				free(*repo_abbrev);

But now the test is useless, because free(NULL) is defined to be a
no-op.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

On Tue, Aug 20, 2013 at 03:40:02PM +0200, Thomas Rast wrote:

Stefan Beller [off-list ref] writes:

quoted

The condition as it is written in that line was most likely intended to
check for the pointer passed to free(), rather than checking for the
'repo_abbrev', which is already checked against being non null at the
beginning of the function.

[...]

quoted

-			if (repo_abbrev)
+			if (*repo_abbrev)
 				free(*repo_abbrev);

But now the test is useless, because free(NULL) is defined to be a
no-op.

Yeah, I think we should just drop the conditional completely.

I am not sure of the complete back-story. The earlier check for
repo_abbrev to be non-NULL was added by 8503ee4, after this check on
free() already existed. So that was when this conditional became
redundant.

But the line right after this one unconditionally assigns to
"*repo_abbrev", so we would always segfault in such a case anyway (which
is what 8503ee4 was fixing).

So I think it was either a misguided "don't pass NULL to free" check
that was simply wrong, or it was an incomplete "make sure repo_abbrev is
not NULL" check. And the first is useless, and the second is now
redundant due to 8503ee4. So it should simply be free().

-Peff

On 08/20/2013 03:40 PM, Thomas Rast wrote:

Stefan Beller [off-list ref] writes:

quoted

The condition as it is written in that line was most likely intended to
check for the pointer passed to free(), rather than checking for the
'repo_abbrev', which is already checked against being non null at the
beginning of the function.

[...]

quoted

-			if (repo_abbrev)
+			if (*repo_abbrev)
 				free(*repo_abbrev);

But now the test is useless, because free(NULL) is defined to be a
no-op.

Yes, indeed. Thanks for reviewing.

Stepping two steps back, I am trying to figure out, what this repo_abrev
thing is doing, as I could find no documentation.

It's passed as a double pointer as declared in mailmap.h:
int read_mailmap(struct string_list *map, char **repo_abbrev);

However grepping for "read_mailmap(" (bracket to prevent finding 
read_mailmap_XXX as often used in mailmap.c itself) 
	grep -nHIirF --exclude-dir=.git -- "read_mailmap("
throughout all the sources I just find one occurence having the
second argument not being 'NULL' and that is in 
	builtin/shortlog.c:212:	read_mailmap(&log->mailmap, &log->common_repo_prefix);

which turns out to be:

void shortlog_init(struct shortlog *log)
{
	memset(log, 0, sizeof(*log));

	read_mailmap(&log->mailmap, &log->common_repo_prefix);
	...

So we're passing there an address, which was just set to zero.
This is the only occurence of passing a value at all and the value
being passed is 0, so the free in the original patch doesn't need 
that check either.

As I am resending the patch, could somebody please explain me
the mechanism of the "# repo-abbrev:" line? Even git itself doesn't
use it in the .mailmap file, but a quick google search shows up only
kernel repositories.

Stefan

The condition as it is written in that line has already been checked
in the beginning of the function, which was introduced in
8503ee4 (2007-05-01, Fix read_mailmap to handle a caller uninterested
in repo abbreviation)

Helped-by: Jeff King [off-list ref]
Helped-by: Thomas Rast [off-list ref]
Signed-off-by: Stefan Beller <redacted>
---
 mailmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mailmap.c b/mailmap.c
index 44614fc..7d5caa6 100644
--- a/mailmap.c
+++ b/mailmap.c

@@ -153,8 +153,7 @@ static void read_mailmap_line(struct string_list *map, char *buffer,
 		if (!strncmp(buffer, abbrev, abblen)) {
 			char *cp;
 
-			if (repo_abbrev)
-				free(*repo_abbrev);
+			free(*repo_abbrev);
 			*repo_abbrev = xmalloc(len);
 
 			for (cp = buffer + abblen; isspace(*cp); cp++)

-- 
1.8.4.rc3.1.gc1ebd90

On Tue, Aug 20, 2013 at 04:17:07PM +0200, Stefan Beller wrote:

Stepping two steps back, I am trying to figure out, what this repo_abrev
thing is doing, as I could find no documentation.

It's meant to abbreviate long pathnames in subject lines. As you noted,
the kernel .mailmap has:

  # repo-abbrev: /pub/scm/linux/kernel/git/

Try "git shortlog" in the kernel and grep for "..." to see its effect.

It is IMHO a misfeature to have it as part of .mailmap, but it is there
for historical reasons. And I think it is not really needed these days
anyway, as the messages created by git-merge are nicer to read in the
first place (and people tend to use nice readable URLs for accessing
one-off git pulls, too).

So we're passing there an address, which was just set to zero.
This is the only occurence of passing a value at all and the value
being passed is 0, so the free in the original patch doesn't need
that check either.

Right. I think the intent was to free a previously found repo-abbrev
line to avoid leaking memory (although arguably, it would make sense to
keep a list and abbreviate all that we find, I don't think anybody cares
anymore for the reasons I stated above).

-Peff

On Tue, Aug 20, 2013 at 04:18:00PM +0200, Stefan Beller wrote:

The condition as it is written in that line has already been checked
in the beginning of the function, which was introduced in
8503ee4 (2007-05-01, Fix read_mailmap to handle a caller uninterested
in repo abbreviation)

Helped-by: Jeff King [off-list ref]
Helped-by: Thomas Rast [off-list ref]
Signed-off-by: Stefan Beller <redacted>
---

This version looks good to me.  Thanks.

-Peff