Coverity CID 1248848 - Leaked Storage Variable.

[araujo]

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.

[ray]

Yup, that one looks correct.

[davide]

LGTM

[marcel]

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.

[araujo]

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.

[araujo]

How about this patch?

uart_subr.diff990 BDownload

[marcel]

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()

[araujo]

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]

Marcelo,

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

[araujo]

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.

[araujo]

Committed: https://svnweb.freebsd.org/base?view=revision&revision=273576

Thank you guys.