* Re: [PATCH] drivers/acpi/ec.c fix a small memory leak
@ 2010-02-02 15:35 ` Alexey Starikovskiy
0 siblings, 0 replies; 11+ messages in thread
From: Alexey Starikovskiy @ 2010-02-02 15:35 UTC (permalink / raw)
To: Darren Jenkins
Cc: Kernel Janitors, Len Brown, linux ACPI, Linux Kernel Mailing List
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.
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 */
>
>
^ permalink raw reply [flat|nested] 11+ 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
-1 siblings, 0 replies; 11+ 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] 11+ messages in thread* Re: [PATCH] drivers/acpi/ec.c fix a small memory leak
@ 2010-02-03 6:12 ` Dan Carpenter
0 siblings, 0 replies; 11+ 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] 11+ messages in thread* Re: [PATCH] drivers/acpi/ec.c fix a small memory leak
@ 2010-02-03 6:12 ` Dan Carpenter
0 siblings, 0 replies; 11+ 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" 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] 11+ messages in thread
* Re: [PATCH] drivers/acpi/ec.c fix a small memory leak
2010-02-02 15:35 ` Alexey Starikovskiy
@ 2010-02-03 11:06 ` Bernd Petrovitsch
-1 siblings, 0 replies; 11+ 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] 11+ messages in thread* Re: [PATCH] drivers/acpi/ec.c fix a small memory leak
@ 2010-02-03 11:06 ` Bernd Petrovitsch
0 siblings, 0 replies; 11+ 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] 11+ messages in thread
* Re: [PATCH] drivers/acpi/ec.c fix a small memory leak
2010-02-02 15:35 ` Alexey Starikovskiy
@ 2010-02-03 11:33 ` Darren Jenkins
-1 siblings, 0 replies; 11+ 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] 11+ messages in thread
* Re: [PATCH] drivers/acpi/ec.c fix a small memory leak
@ 2010-02-03 11:33 ` Darren Jenkins
0 siblings, 0 replies; 11+ 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] 11+ messages in thread