* [PATCH] staging: android: ion: hisilicon: Use pointer to memory being allocated as the sizeof argument @ 2016-09-21 15:41 sayli karnik 2016-09-21 16:15 ` Greg Kroah-Hartman 0 siblings, 1 reply; 5+ messages in thread From: sayli karnik @ 2016-09-21 15:41 UTC (permalink / raw) To: outreachy-kernel Cc: Laura Abbott, Sumit Semwal, Greg Kroah-Hartman, Arve Hjønnevåg, Riley Andrews This patch finds cases where the argument to sizeof is wrong in memory allocation functions by checking the type of the allocated memory when it is a double pointer and ensuring the sizeof argument takes a pointer to the memory being allocated. Done using coccinelle: @@ type T; T **x; @@ x = <+...sizeof( - T + *x )...+> Signed-off-by: sayli karnik <karniksayli1995@gmail.com> --- drivers/staging/android/ion/hisilicon/hi6220_ion.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/android/ion/hisilicon/hi6220_ion.c b/drivers/staging/android/ion/hisilicon/hi6220_ion.c index 659aa71..9f9b244 100644 --- a/drivers/staging/android/ion/hisilicon/hi6220_ion.c +++ b/drivers/staging/android/ion/hisilicon/hi6220_ion.c @@ -57,7 +57,7 @@ static int hi6220_ion_probe(struct platform_device *pdev) return PTR_ERR(ipdev->data); ipdev->heaps = devm_kzalloc(&pdev->dev, - sizeof(struct ion_heap)*ipdev->data->nr, + sizeof(*ipdev->heaps) * ipdev->data->nr, GFP_KERNEL); if (!ipdev->heaps) { ion_destroy_platform_data(ipdev->data); -- 2.7.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: android: ion: hisilicon: Use pointer to memory being allocated as the sizeof argument 2016-09-21 15:41 [PATCH] staging: android: ion: hisilicon: Use pointer to memory being allocated as the sizeof argument sayli karnik @ 2016-09-21 16:15 ` Greg Kroah-Hartman 2016-09-21 16:31 ` sayli karnik 0 siblings, 1 reply; 5+ messages in thread From: Greg Kroah-Hartman @ 2016-09-21 16:15 UTC (permalink / raw) To: sayli karnik Cc: outreachy-kernel, Laura Abbott, Sumit Semwal, Arve Hjønnevåg, Riley Andrews On Wed, Sep 21, 2016 at 09:11:14PM +0530, sayli karnik wrote: > This patch finds cases where the argument to sizeof is wrong in memory > allocation functions by checking the type of the allocated memory when it is a > double pointer and ensuring the sizeof argument takes a pointer to the memory > being allocated. Are you sure? > Done using coccinelle: > @@ > type T; > T **x; > @@ > > x = > <+...sizeof( > - T > + *x > )...+> > > Signed-off-by: sayli karnik <karniksayli1995@gmail.com> > --- > drivers/staging/android/ion/hisilicon/hi6220_ion.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/android/ion/hisilicon/hi6220_ion.c b/drivers/staging/android/ion/hisilicon/hi6220_ion.c > index 659aa71..9f9b244 100644 > --- a/drivers/staging/android/ion/hisilicon/hi6220_ion.c > +++ b/drivers/staging/android/ion/hisilicon/hi6220_ion.c > @@ -57,7 +57,7 @@ static int hi6220_ion_probe(struct platform_device *pdev) > return PTR_ERR(ipdev->data); > > ipdev->heaps = devm_kzalloc(&pdev->dev, > - sizeof(struct ion_heap)*ipdev->data->nr, > + sizeof(*ipdev->heaps) * ipdev->data->nr, Do we really want to allocate an array of pointers, or an array of structures? Which is it, as it's not obvious... thanks, greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: android: ion: hisilicon: Use pointer to memory being allocated as the sizeof argument 2016-09-21 16:15 ` Greg Kroah-Hartman @ 2016-09-21 16:31 ` sayli karnik 2016-09-21 16:36 ` [Outreachy kernel] " Julia Lawall 2016-09-21 16:41 ` Greg Kroah-Hartman 0 siblings, 2 replies; 5+ messages in thread From: sayli karnik @ 2016-09-21 16:31 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: outreachy-kernel, Laura Abbott, Sumit Semwal, Arve Hjønnevåg, Riley Andrews On Wed, Sep 21, 2016 at 9:45 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Wed, Sep 21, 2016 at 09:11:14PM +0530, sayli karnik wrote: >> This patch finds cases where the argument to sizeof is wrong in memory >> allocation functions by checking the type of the allocated memory when it is a >> double pointer and ensuring the sizeof argument takes a pointer to the memory >> being allocated. > > Are you sure? > This was detected by scripts/coccinelle/misc/badty.cocci, which specifies that a false positive only arises when the sizeof argument is not used in constructing the return value. Also the driver builds without errors. What do you think? > >> Done using coccinelle: >> @@ >> type T; >> T **x; >> @@ >> >> x = >> <+...sizeof( >> - T >> + *x >> )...+> >> >> Signed-off-by: sayli karnik <karniksayli1995@gmail.com> >> --- >> drivers/staging/android/ion/hisilicon/hi6220_ion.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/staging/android/ion/hisilicon/hi6220_ion.c b/drivers/staging/android/ion/hisilicon/hi6220_ion.c >> index 659aa71..9f9b244 100644 >> --- a/drivers/staging/android/ion/hisilicon/hi6220_ion.c >> +++ b/drivers/staging/android/ion/hisilicon/hi6220_ion.c >> @@ -57,7 +57,7 @@ static int hi6220_ion_probe(struct platform_device *pdev) >> return PTR_ERR(ipdev->data); >> >> ipdev->heaps = devm_kzalloc(&pdev->dev, >> - sizeof(struct ion_heap)*ipdev->data->nr, >> + sizeof(*ipdev->heaps) * ipdev->data->nr, > > Do we really want to allocate an array of pointers, or an array of > structures? Which is it, as it's not obvious... > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH] staging: android: ion: hisilicon: Use pointer to memory being allocated as the sizeof argument 2016-09-21 16:31 ` sayli karnik @ 2016-09-21 16:36 ` Julia Lawall 2016-09-21 16:41 ` Greg Kroah-Hartman 1 sibling, 0 replies; 5+ messages in thread From: Julia Lawall @ 2016-09-21 16:36 UTC (permalink / raw) To: sayli karnik Cc: Greg Kroah-Hartman, outreachy-kernel, Laura Abbott, Sumit Semwal, Arve Hjønnevåg, Riley Andrews On Wed, 21 Sep 2016, sayli karnik wrote: > On Wed, Sep 21, 2016 at 9:45 PM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Wed, Sep 21, 2016 at 09:11:14PM +0530, sayli karnik wrote: > >> This patch finds cases where the argument to sizeof is wrong in memory > >> allocation functions by checking the type of the allocated memory when it is a > >> double pointer and ensuring the sizeof argument takes a pointer to the memory > >> being allocated. > > > > Are you sure? > > > This was detected by scripts/coccinelle/misc/badty.cocci, which > specifies that a false positive only arises when the sizeof argument > is not used in constructing the return value. Also the driver builds > without errors. What do you think? Neither of these are convincing argument. For example, if you replaced the sizeof by 27, it would also build. And Coccinelle doesn't know anything aboout the intent of the code. So you would have to check it carefully. > > > > >> Done using coccinelle: > >> @@ > >> type T; > >> T **x; > >> @@ > >> > >> x = > >> <+...sizeof( > >> - T > >> + *x This change looks appropriate for a single pointer case, not a double pointer case. Ie where x has type T *x. Check the code again and think about what types are involved. julia > >> )...+> > >> > >> Signed-off-by: sayli karnik <karniksayli1995@gmail.com> > >> --- > >> drivers/staging/android/ion/hisilicon/hi6220_ion.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/staging/android/ion/hisilicon/hi6220_ion.c b/drivers/staging/android/ion/hisilicon/hi6220_ion.c > >> index 659aa71..9f9b244 100644 > >> --- a/drivers/staging/android/ion/hisilicon/hi6220_ion.c > >> +++ b/drivers/staging/android/ion/hisilicon/hi6220_ion.c > >> @@ -57,7 +57,7 @@ static int hi6220_ion_probe(struct platform_device *pdev) > >> return PTR_ERR(ipdev->data); > >> > >> ipdev->heaps = devm_kzalloc(&pdev->dev, > >> - sizeof(struct ion_heap)*ipdev->data->nr, > >> + sizeof(*ipdev->heaps) * ipdev->data->nr, > > > > Do we really want to allocate an array of pointers, or an array of > > structures? Which is it, as it's not obvious... > > > > thanks, > > > > greg k-h > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To post to this group, send email to outreachy-kernel@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAKG5xWgV-0cKboce8i8LbLB7kBQPMYs_NjcredvggXX6Qb%3DUGA%40mail.gmail.com. > For more options, visit https://groups.google.com/d/optout. > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: android: ion: hisilicon: Use pointer to memory being allocated as the sizeof argument 2016-09-21 16:31 ` sayli karnik 2016-09-21 16:36 ` [Outreachy kernel] " Julia Lawall @ 2016-09-21 16:41 ` Greg Kroah-Hartman 1 sibling, 0 replies; 5+ messages in thread From: Greg Kroah-Hartman @ 2016-09-21 16:41 UTC (permalink / raw) To: sayli karnik Cc: outreachy-kernel, Laura Abbott, Sumit Semwal, Arve Hjønnevåg, Riley Andrews On Wed, Sep 21, 2016 at 10:01:09PM +0530, sayli karnik wrote: > On Wed, Sep 21, 2016 at 9:45 PM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Wed, Sep 21, 2016 at 09:11:14PM +0530, sayli karnik wrote: > >> This patch finds cases where the argument to sizeof is wrong in memory > >> allocation functions by checking the type of the allocated memory when it is a > >> double pointer and ensuring the sizeof argument takes a pointer to the memory > >> being allocated. > > > > Are you sure? > > > This was detected by scripts/coccinelle/misc/badty.cocci, which > specifies that a false positive only arises when the sizeof argument > is not used in constructing the return value. Also the driver builds > without errors. What do you think? building without errors is not the issue here at all, it's a number that is being passed in, you could replace that statement with "42" and it would still build. But would it work when running? That's the question here :) Please look at the surrounding code and see if it is correct or not, somehow it is working as-is, right? Are we just allocating too much memory? Pointer math is tricky at times, I can understand coccinelle scripts getting confused here... thanks, greg k-h ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-21 16:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-21 15:41 [PATCH] staging: android: ion: hisilicon: Use pointer to memory being allocated as the sizeof argument sayli karnik 2016-09-21 16:15 ` Greg Kroah-Hartman 2016-09-21 16:31 ` sayli karnik 2016-09-21 16:36 ` [Outreachy kernel] " Julia Lawall 2016-09-21 16:41 ` Greg Kroah-Hartman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.