Page MenuHomeFreeBSD

Coverity CID 1248848 - Leaked Storage Variable.
ClosedPublic

Authored by araujo on Oct 21 2014, 6:08 AM.

Details

Summary

Coverity reports a leaked storage variable.
I do believe just set spec to NULL will cover this issue.

This approach was suggested by kevlo@.
I have sent another patch that is pretty much wrong.

Diff Detail

Repository
rS FreeBSD src repository
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

araujo updated this revision to Diff 2068.Oct 21 2014, 6:08 AM
araujo retitled this revision from to Coverity CID 1248848 - Leaked Storage Variable..
araujo updated this object.
araujo edited the test plan for this revision. (Show Details)
ray accepted this revision.Oct 21 2014, 12:36 PM
ray edited edge metadata.

Yup, that one looks correct.

This revision is now accepted and ready to land.Oct 21 2014, 12:36 PM
davide accepted this revision.Oct 21 2014, 12:47 PM
davide edited edge metadata.
marcel added a subscriber: marcel.Oct 21 2014, 4:47 PM

As replied to araujo before I created my account:

I think the change is wrong. spec doesn't point to the beginning of the string after the first iteration. So you can't free spec anymore. In order to properly free the string with freeenv, we need to save the pointer returned from getenv.

Also: we probably want to free the string in the normal case as well as the error case. You just fixed the error case.

In D981#8, @marcel wrote:

As replied to araujo before I created my account:
I think the change is wrong. spec doesn't point to the beginning of the string after the first iteration. So you can't free spec anymore. In order to properly free the string with freeenv, we need to save the pointer returned from getenv.

Yes, you are right, at line 276 it will iterate and can't be freeenv anymore, but it can be set to NULL before the return.

Also: we probably want to free the string in the normal case as well as the error case. You just fixed the error case.

I'm gonna make another patch.
Thank all you guys by the prompt reply.

How about this patch?

marcel edited edge metadata.Oct 22 2014, 4:40 AM

Setting spec to NULL does nothing. The compiler will effectively remove it as being dead-code.

Also: the CID is about a memory leak. The leak is caused by kern_getenv() doing a strdup() in certain conditions and in order to free the memory allocated by strdup(), one must call kern_freeenv(). Setting spec to NULL does not fix the problem of not calling kern_freeenv()

In D981#13, @marcel wrote:

Setting spec to NULL does nothing. The compiler will effectively remove it as being dead-code.
Also: the CID is about a memory leak. The leak is caused by kern_getenv() doing a strdup() in certain conditions and in order to free the memory allocated by strdup(), one must call kern_freeenv(). Setting spec to NULL does not fix the problem of not calling kern_freeenv()

OK, I'm gonna try another approach. Sorry for the noise.

kevlo edited edge metadata.Oct 24 2014, 1:32 AM

Marcelo,

I sent marcel@ the diff for review yesterday and he's ok with it.
https://people.freebsd.org/~kevlo/patch-uart_subr.c

In D981#15, @kevlo wrote:

Marcelo,
I sent marcel@ the diff for review yesterday and he's ok with it.
https://people.freebsd.org/~kevlo/patch-uart_subr.c

Hello Kevlo,

Thank you very much for the patch. I will commit it this afternoon.