* Re: [PATCH] drivers/acpi/ec.c fix a small memory leak
2010-02-02 15:35 ` Alexey Starikovskiy
@ 2010-02-03 6:12 ` Dan Carpenter
2010-02-03 11:06 ` Bernd Petrovitsch
2010-02-03 11:33 ` Darren Jenkins
2 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2010-02-03 6:12 UTC (permalink / raw)
To: Alexey Starikovskiy
Cc: Darren Jenkins, Kernel Janitors, Len Brown, linux ACPI,
Linux Kernel Mailing List
On Tue, Feb 02, 2010 at 06:35:44PM +0300, Alexey Starikovskiy wrote:
> NAK
>
> saved_ec is allocated if flag EC_FLAGS_VALIDATE_ECDT is true.
> EC_FLAGS_SKIP_DSDT_SCAN have no sense in such case, thus this new code path
> is never executed.
> On the other hand, unconditionally freeing pointer, which is might be NULL, is not
> right either.
As near as I can tell you are right that the code cannot leak memory. But Darren
is right to think that it's calling kfree() on a NULL pointer is OK. His patch does
not introduce any errors.
The logic here is basically spaghetti. Every person who looks at it is going to
think it's a leak. Lots of people are going to look at it, because the static
checkers all think it's a leak too. Someone could send a cleanup patch that makes it
readable.
regards,
dan carpenter
> So, this patch introduced more problems as it tries to solve...
>
> Regards,
> Alex.
>
> Darren Jenkins пишет:
> > Plug a very small leak in acpi_ec_ecdt_probe()
> >
> > Coverity CID: 13319
> >
> > Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com>
> >
> > diff --git drivers/acpi/ec.c drivers/acpi/ec.c
> > index d6471bb..13061dc 100644
> > --- drivers/acpi/ec.c
> > +++ drivers/acpi/ec.c
> > @@ -1009,8 +1009,10 @@ int __init acpi_ec_ecdt_probe(void)
> > /* fall through */
> > }
> >
> > - if (EC_FLAGS_SKIP_DSDT_SCAN)
> > + if (EC_FLAGS_SKIP_DSDT_SCAN) {
> > + kfree(saved_ec);
> > return -ENODEV;
> > + }
> >
> > /* This workaround is needed only on some broken machines,
> > * which require early EC, but fail to provide ECDT */
> >
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drivers/acpi/ec.c fix a small memory leak
2010-02-02 15:35 ` Alexey Starikovskiy
2010-02-03 6:12 ` Dan Carpenter
@ 2010-02-03 11:06 ` Bernd Petrovitsch
2010-02-03 11:33 ` Darren Jenkins
2 siblings, 0 replies; 5+ messages in thread
From: Bernd Petrovitsch @ 2010-02-03 11:06 UTC (permalink / raw)
To: Alexey Starikovskiy
Cc: Darren Jenkins, Kernel Janitors, Len Brown, linux ACPI,
Linux Kernel Mailing List
On Die, 2010-02-02 at 18:35 +0300, Alexey Starikovskiy wrote:
> NAK
>
> saved_ec is allocated if flag EC_FLAGS_VALIDATE_ECDT is true.
> EC_FLAGS_SKIP_DSDT_SCAN have no sense in such case, thus this new code path
> is never executed.
> On the other hand, unconditionally freeing pointer, which is might be NULL, is not
> right either.
Why that?
"kfree(NULL);" works (and in the user-space "free(NULL)" too FWIW).
> So, this patch introduced more problems as it tries to solve...
Bernd
--
Bernd Petrovitsch Email : bernd@petrovitsch.priv.at
LUGA : http://www.luga.at
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drivers/acpi/ec.c fix a small memory leak
2010-02-02 15:35 ` Alexey Starikovskiy
2010-02-03 6:12 ` Dan Carpenter
2010-02-03 11:06 ` Bernd Petrovitsch
@ 2010-02-03 11:33 ` Darren Jenkins
2 siblings, 0 replies; 5+ messages in thread
From: Darren Jenkins @ 2010-02-03 11:33 UTC (permalink / raw)
To: Alexey Starikovskiy
Cc: Kernel Janitors, Len Brown, linux ACPI, Linux Kernel Mailing List
Hello Alexey,
On Wed, Feb 3, 2010 at 2:35 AM, Alexey Starikovskiy
<astarikovskiy@suse.de> wrote:
> NAK
>
> saved_ec is allocated if flag EC_FLAGS_VALIDATE_ECDT is true.
> EC_FLAGS_SKIP_DSDT_SCAN have no sense in such case, thus this new code path
> is never executed.
>From what I can tell this is not guaranteed by the code, it is relying
on DMI data not matching two of the stings at once.
However I don't know anything about DMI, so if you say this makes no
sense and will never happen I am happy.
I will mark this as ignore in the Coverity databse.
> On the other hand, unconditionally freeing pointer, which is might be NULL, is not
> right either.
Nowadays this is the preferred method actually.
> Regards,
> Alex.
Thanks for the feedback.
Darren J
^ permalink raw reply [flat|nested] 5+ messages in thread