1245441 - [Coverity 220566] Free a pointer

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Paul Bignier]

Attachment: 0001-Coverity-220566-Free-a-pointer.patch

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20160127060627

Steps to reproduce:

Coverity's CID: 220566

Comment 1

[Kai Engert (:KaiE:)]

Comment on attachment 8715232 [details] [diff] [review]
0001-Coverity-220566-Free-a-pointer.patch

It seems that it's possible that no value is assigned to variable phy.
Then you'd attempt to free a random pointer.

You could probably solve that by initializing phy to NULL at declaration time.

Comment 2

[Paul Bignier]

Attachment: 0001-Coverity-220566-Free-a-pointer.patch

Comment 3

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/9448cd7cf830

Comment 4

[Kai Engert (:KaiE:)]

I've backed this out.
https://hg.mozilla.org/projects/nss/rev/ccaddca3b86b

I believe it is responsible for a buildbot bustage.

I should have reviewed the patch more carefully.
Apparently the code may transfer ownership of the allocated phy pointer to a list, and we must not free that pointer when that happens.

  it->data = (unsigned char *) phy;

It might be sufficient to set "phy=NULL" after that transfer. But I'd like to ask you to please investigate the code more carefully.

Comment 5

[Paul Bignier]

Attachment: 0001-Coverity-220566-Free-a-pointer.patch

'phy' should indeed be freed only if 'it' didn't manage to be allocated.
No need to initialize 'it' because it is given a value right after 'phy'.

Poor indentation got me confused, I didn't see we were in the while loop.

Comment 6

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8716871 [details] [diff] [review]
0001-Coverity-220566-Free-a-pointer.patch

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

lgtm

Comment 7

[Franziskus Kiefer [:franziskus]]

could you make a patch against NSS trunk?

Comment 8

[Paul Bignier]

Attachment: 0001-Coverity-220566-Free-a-pointer.patch

> could you make a patch against NSS trunk?

Hi, sure, here it is.

The content is the same as the previous patch, I hope it's all right for you because I'm having errors when applying it ("space before tab in indent" error).

Regards,
Paul

Comment 9

[Franziskus Kiefer [:franziskus]]

Attachment: bug1245441.patch

thanks, though I landed the clang-format patch earlier today. Had to change your patch again.

Comment 10

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/7208817c1958