1218792 - [CID 222162][CID 222161] overwrite_var: Overwriting subiter in subiter = Pk11Install_ListIter_new

Status: RESOLVED FIXED

Description

[Franziskus Kiefer [:franziskus] (Away until October 2019)]

Coverity complains that subiter is overwritten here and thus leaks storage.

> subiter = Pk11Install_ListIter_new(subpair->list);
> [...]
> Pk11Install_ListIter_delete(subiter);
> subiter = NULL;

I'm not sure what Pk11Install_ListIter_delete is supposed to do (delete probably), but it only sets 

> _this->list=NULL;
> _this->current=NULL;

without freeing anything. To make this work properly we have to free _this. We can also pull the subiter=NULL in the delete function.

Comment 1

[Franziskus Kiefer [:franziskus] (Away until October 2019)]

Created attachment 8679460 [details] [diff] [review]
don't leak the iter

moved the free and null in the delete function

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8679460 [details] [diff] [review]
don't leak the iter

Review of attachment 8679460 [details] [diff] [review]:
-----------------------------------------------------------------

This might have been a double-free in the past, but removing the NULL assignments means that you have a new type of double-free.

::: cmd/modutil/install-ds.c
@@ +1334,5 @@
>  {
>  	_this->list=NULL;
>  	_this->current=NULL;
> +	PR_Free(_this);
> +	_this=NULL;

This isn't going to do what you think it does.

Comment 3

[Franziskus Kiefer [:franziskus] (Away until October 2019)]

Created attachment 8680010 [details] [diff] [review]
don't leak the iter

uh, right. how about this?

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8680010 [details] [diff] [review]
don't leak the iter

Review of attachment 8680010 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM  Deferring the commit.

Comment 5

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/fca456c49a53