[PATCH 1/2] ui-stats: free without condition

[PATCH 1/2] ui-stats: free without condition

[list]

From: Christian Hesse <mail at eworm.de>

xstrdup() returns allocated memory or NULL. It's safe to call free()
without condition.

Coverity-Id 13839 is kind of false posivtive, but this should fix it
nevertheless.

Coverity-Id: 13839
Signed-off-by: Christian Hesse <mail at eworm.de>
---
 ui-stats.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/ui-stats.c b/ui-stats.c
index 74ce0f7..02335e7 100644
--- a/ui-stats.c
+++ b/ui-stats.c
@@ -180,8 +180,7 @@ static void add_commit(struct string_list *authors, struct commit *commit,
 	author = string_list_insert(authors, tmp);
 	if (!author->util)
 		author->util = xcalloc(1, sizeof(struct authorstat));
-	else
-		free(tmp);
+	free(tmp);
 	authorstat = author->util;
 	items = &authorstat->list;
 	t = info->committer_date;
@@ -189,8 +188,7 @@ static void add_commit(struct string_list *authors, struct commit *commit,
 	period->trunc(date);
 	tmp = xstrdup(period->pretty(date));
 	item = string_list_insert(items, tmp);
-	if (item->util)
-		free(tmp);
+	free(tmp);
 	item->util++;
 	authorstat->total++;
 	cgit_free_commitinfo(info);
-- 
2.6.1

[PATCH 2/2] ui-stats: do not duplicate string

[list]

From: Christian Hesse <mail at eworm.de>

string_list_insert() expects (const char*), so no need to duplicate and
free the string.

Signed-off-by: Christian Hesse <mail at eworm.de>
---
 ui-stats.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/ui-stats.c b/ui-stats.c
index 02335e7..26473de 100644
--- a/ui-stats.c
+++ b/ui-stats.c
@@ -171,24 +171,19 @@ static void add_commit(struct string_list *authors, struct commit *commit,
 	struct string_list_item *author, *item;
 	struct authorstat *authorstat;
 	struct string_list *items;
-	char *tmp;
 	struct tm *date;
 	time_t t;
 
 	info = cgit_parse_commit(commit);
-	tmp = xstrdup(info->author);
-	author = string_list_insert(authors, tmp);
+	author = string_list_insert(authors, info->author);
 	if (!author->util)
 		author->util = xcalloc(1, sizeof(struct authorstat));
-	free(tmp);
 	authorstat = author->util;
 	items = &authorstat->list;
 	t = info->committer_date;
 	date = gmtime(&t);
 	period->trunc(date);
-	tmp = xstrdup(period->pretty(date));
-	item = string_list_insert(items, tmp);
-	free(tmp);
+	item = string_list_insert(items, period->pretty(date));
 	item->util++;
 	authorstat->total++;
 	cgit_free_commitinfo(info);
-- 
2.6.1

[PATCH 1/2] ui-stats: free without condition

[john]

On Mon, Oct 12, 2015 at 10:59:34AM +0200, Christian Hesse wrote:
> From: Christian Hesse <mail at eworm.de>
> 
> xstrdup() returns allocated memory or NULL. It's safe to call free()
> without condition.
> 
> Coverity-Id 13839 is kind of false posivtive, but this should fix it
> nevertheless.
> 
> Coverity-Id: 13839
> Signed-off-by: Christian Hesse <mail at eworm.de>
> ---
>  ui-stats.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

This is wrong - we don't have strdup_strings set in the string_list so
it takes ownership of the pointer.  The test on item->util is used as a
proxy testing if the entry is newly added to the list (in which case it
has taken ownership of the string) or not (in which case we must free
the string).

> diff --git a/ui-stats.c b/ui-stats.c
> index 74ce0f7..02335e7 100644
> --- a/ui-stats.c
> +++ b/ui-stats.c
> @@ -180,8 +180,7 @@ static void add_commit(struct string_list *authors, struct commit *commit,
>  	author = string_list_insert(authors, tmp);
>  	if (!author->util)
>  		author->util = xcalloc(1, sizeof(struct authorstat));
> -	else
> -		free(tmp);
> +	free(tmp);
>  	authorstat = author->util;
>  	items = &authorstat->list;
>  	t = info->committer_date;
> @@ -189,8 +188,7 @@ static void add_commit(struct string_list *authors, struct commit *commit,
>  	period->trunc(date);
>  	tmp = xstrdup(period->pretty(date));
>  	item = string_list_insert(items, tmp);
> -	if (item->util)
> -		free(tmp);
> +	free(tmp);
>  	item->util++;
>  	authorstat->total++;
>  	cgit_free_commitinfo(info);
> -- 
> 2.6.1

[PATCH 2/2] ui-stats: do not duplicate string

[john]

On Mon, Oct 12, 2015 at 10:59:35AM +0200, Christian Hesse wrote:
> From: Christian Hesse <mail at eworm.de>
> 
> string_list_insert() expects (const char*), so no need to duplicate and
> free the string.
> 
> Signed-off-by: Christian Hesse <mail at eworm.de>
> ---
>  ui-stats.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)

This has the same problem as the previous patch.

> diff --git a/ui-stats.c b/ui-stats.c
> index 02335e7..26473de 100644
> --- a/ui-stats.c
> +++ b/ui-stats.c
> @@ -171,24 +171,19 @@ static void add_commit(struct string_list *authors, struct commit *commit,
>  	struct string_list_item *author, *item;
>  	struct authorstat *authorstat;
>  	struct string_list *items;
> -	char *tmp;
>  	struct tm *date;
>  	time_t t;
>  
>  	info = cgit_parse_commit(commit);
> -	tmp = xstrdup(info->author);
> -	author = string_list_insert(authors, tmp);
> +	author = string_list_insert(authors, info->author);
>  	if (!author->util)
>  		author->util = xcalloc(1, sizeof(struct authorstat));
> -	free(tmp);
>  	authorstat = author->util;
>  	items = &authorstat->list;
>  	t = info->committer_date;
>  	date = gmtime(&t);
>  	period->trunc(date);
> -	tmp = xstrdup(period->pretty(date));
> -	item = string_list_insert(items, tmp);
> -	free(tmp);
> +	item = string_list_insert(items, period->pretty(date));
>  	item->util++;
>  	authorstat->total++;
>  	cgit_free_commitinfo(info);
> -- 
> 2.6.1

[PATCH 1/2] ui-stats: free without condition

[list]

John Keeping <john at keeping.me.uk> on Mon, 2015/10/12 10:10:
> On Mon, Oct 12, 2015 at 10:59:34AM +0200, Christian Hesse wrote:
> > From: Christian Hesse <mail at eworm.de>
> > 
> > xstrdup() returns allocated memory or NULL. It's safe to call free()
> > without condition.
> > 
> > Coverity-Id 13839 is kind of false posivtive, but this should fix it
> > nevertheless.
> > 
> > Coverity-Id: 13839
> > Signed-off-by: Christian Hesse <mail at eworm.de>
> > ---
> >  ui-stats.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)  
> 
> This is wrong - we don't have strdup_strings set in the string_list so
> it takes ownership of the pointer.  The test on item->util is used as a
> proxy testing if the entry is newly added to the list (in which case it
> has taken ownership of the string) or not (in which case we must free
> the string).

My assumption was that string_list_insert() handles this itself and
duplicates the string. Then please ignore the patches! :D
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Chris           get my mail address:    */=0;b=c[a++];)
putchar(b-1/(/*               gcc -o sig sig.c && ./sig    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20151012/1b46108f/attachment.asc>

[PATCH 1/2] ui-stats: free without condition

[list]

Christian Hesse <list at eworm.de> on Mon, 2015/10/12 11:14:
> John Keeping <john at keeping.me.uk> on Mon, 2015/10/12 10:10:
> > On Mon, Oct 12, 2015 at 10:59:34AM +0200, Christian Hesse wrote:  
> > > From: Christian Hesse <mail at eworm.de>
> > > 
> > > xstrdup() returns allocated memory or NULL. It's safe to call free()
> > > without condition.
> > > 
> > > Coverity-Id 13839 is kind of false posivtive, but this should fix it
> > > nevertheless.
> > > 
> > > Coverity-Id: 13839
> > > Signed-off-by: Christian Hesse <mail at eworm.de>
> > > ---
> > >  ui-stats.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)    
> > 
> > This is wrong - we don't have strdup_strings set in the string_list so
> > it takes ownership of the pointer.  The test on item->util is used as a
> > proxy testing if the entry is newly added to the list (in which case it
> > has taken ownership of the string) or not (in which case we must free
> > the string).  
> 
> My assumption was that string_list_insert() handles this itself and
> duplicates the string. Then please ignore the patches! :D

Does it make sense to set

authors.strdup_strings = 1;
items.strdup_strings = 1;

and let string_list_insert() make the work?
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Chris           get my mail address:    */=0;b=c[a++];)
putchar(b-1/(/*               gcc -o sig sig.c && ./sig    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20151012/cef77ba7/attachment-0001.asc>

[PATCH 1/2] ui-stats: free without condition

[john]

On Mon, Oct 12, 2015 at 08:29:20PM +0200, Christian Hesse wrote:
> Christian Hesse <list at eworm.de> on Mon, 2015/10/12 11:14:
> > John Keeping <john at keeping.me.uk> on Mon, 2015/10/12 10:10:
> > > On Mon, Oct 12, 2015 at 10:59:34AM +0200, Christian Hesse wrote:  
> > > > From: Christian Hesse <mail at eworm.de>
> > > > 
> > > > xstrdup() returns allocated memory or NULL. It's safe to call free()
> > > > without condition.
> > > > 
> > > > Coverity-Id 13839 is kind of false posivtive, but this should fix it
> > > > nevertheless.
> > > > 
> > > > Coverity-Id: 13839
> > > > Signed-off-by: Christian Hesse <mail at eworm.de>
> > > > ---
> > > >  ui-stats.c | 6 ++----
> > > >  1 file changed, 2 insertions(+), 4 deletions(-)    
> > > 
> > > This is wrong - we don't have strdup_strings set in the string_list so
> > > it takes ownership of the pointer.  The test on item->util is used as a
> > > proxy testing if the entry is newly added to the list (in which case it
> > > has taken ownership of the string) or not (in which case we must free
> > > the string).  
> > 
> > My assumption was that string_list_insert() handles this itself and
> > duplicates the string. Then please ignore the patches! :D
> 
> Does it make sense to set
> 
> authors.strdup_strings = 1;
> items.strdup_strings = 1;
> 
> and let string_list_insert() make the work?

Personally, I'd say it's not worth the churn.  I don't think Coverity is
complaining about this anyway is it?  The only problem I see in
ui-stats.c is that we use item->util as a counter not a pointer.  I'm
not sure if casting it to uintptr_t will be enough to persuade Coverity
that we know what we're doing.