| Message ID | 9b1e7330-f4f6-47f8-a568-eaea1624bb6f@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: property: Adjustments for three function implementations | expand |
On 2023-12-26 10:38, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Tue, 26 Dec 2023 08:44:37 +0100 > > The kfree() function was called in one case by the > drm_property_create() function during error handling > even if the passed data structure member contained a null pointer. > This issue was detected by using the Coccinelle software. > > Thus use another label. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/gpu/drm/drm_property.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c > index 596272149a35..3440f4560e6e 100644 > --- a/drivers/gpu/drm/drm_property.c > +++ b/drivers/gpu/drm/drm_property.c > @@ -117,7 +117,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, > property->values = kcalloc(num_values, sizeof(uint64_t), > GFP_KERNEL); > if (!property->values) > - goto fail; > + goto free_property; > } > > ret = drm_mode_object_add(dev, &property->base, DRM_MODE_OBJECT_PROPERTY); > @@ -135,6 +135,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, > return property; > fail: > kfree(property->values); > +free_property: > kfree(property); > return NULL; > } > -- > 2.43.0 > This change is pointless at best, kfree(NULL) works fine. Out of curiosity, what exactly did Coccinelle report?
>> The kfree() function was called in one case by the >> drm_property_create() function during error handling >> even if the passed data structure member contained a null pointer. >> This issue was detected by using the Coccinelle software. >> >> Thus use another label. … >> +++ b/drivers/gpu/drm/drm_property.c >> @@ -117,7 +117,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, >> property->values = kcalloc(num_values, sizeof(uint64_t), >> GFP_KERNEL); >> if (!property->values) >> - goto fail; >> + goto free_property; >> } >> >> ret = drm_mode_object_add(dev, &property->base, DRM_MODE_OBJECT_PROPERTY); >> @@ -135,6 +135,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, >> return property; >> fail: >> kfree(property->values); >> +free_property: >> kfree(property); >> return NULL; >> } … > This change is pointless at best, kfree(NULL) works fine. * Would you interpret such a special function call as redundant? * Do you find advices applicable from another information source also for this function implementation? https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources > Out of curiosity, what exactly did Coccinelle report? Some SmPL scripts from my own selection tend to point questionable implementation details out. Regards, Markus
On 2024-01-03 17:24, Markus Elfring wrote: > >> Out of curiosity, what exactly did Coccinelle report? > > Some SmPL scripts from my own selection tend to point questionable implementation details out. That doesn't answer my question. Without seeing the actual Coccinelle report, I'll assume that it didn't actually call for this change.
>>> Out of curiosity, what exactly did Coccinelle report? >> >> Some SmPL scripts from my own selection tend to point questionable implementation details out. > > That doesn't answer my question. It should. > Without seeing the actual Coccinelle report, There is no “official” report according to the discussed patch which is triggered by known advices for the application of labels in goto chains. > I'll assume that it didn't actually call for this change. Software development opinions are evolving accordingly. Regards, Markus
On 2024-01-03 19:08, Markus Elfring wrote: > >> Without seeing the actual Coccinelle report, > > There is no “official” report according to the discussed patch which is triggered > by known advices for the application of labels in goto chains. The commit log says: This issue was detected by using the Coccinelle software. Either that's inaccurate then, or you should be able to provide the corresponding output from Coccinelle.
> The commit log says: > > This issue was detected by using the Coccinelle software. > > Either that's inaccurate then, No. > or you should be able to provide the corresponding output from Coccinelle. Do you find data (like the following) more helpful for the adjustment of affected implementation details? Markus_Elfring@Sonne:…/Projekte/Linux/next-analyses> LANG=C git status && spatch …/Projekte/Coccinelle/janitor/show_jumps_to_kfree_with_null_pointer.cocci drivers/gpu/drm/drm_property.c HEAD detached at next-20240104 … @@ -114,9 +114,6 @@ struct drm_property *drm_property_create property->dev = dev; if (num_values) { - property->values = kcalloc(num_values, sizeof(uint64_t), - GFP_KERNEL); - if (!property->values) goto fail; } @@ -133,8 +130,6 @@ struct drm_property *drm_property_create list_add_tail(&property->head, &dev->mode_config.property_list); return property; -fail: - kfree(property->values); kfree(property); return NULL; } How do you think about to extend the application of script variants for the semantic patch language? Regards, Markus
On Wed, Jan 03, 2024 at 06:18:13PM +0100, Michel Dänzer wrote: > On 2024-01-03 17:24, Markus Elfring wrote: > > > >> Out of curiosity, what exactly did Coccinelle report? > > > > Some SmPL scripts from my own selection tend to point questionable implementation details out. > > That doesn't answer my question. > > Without seeing the actual Coccinelle report, I'll assume that it didn't actually call for this change. This isn't one of the Coccinelle scripts which ship with the kernel, it's something that Markus wrote himself. regards, dan carpenter
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index 596272149a35..3440f4560e6e 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -117,7 +117,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, property->values = kcalloc(num_values, sizeof(uint64_t), GFP_KERNEL); if (!property->values) - goto fail; + goto free_property; } ret = drm_mode_object_add(dev, &property->base, DRM_MODE_OBJECT_PROPERTY); @@ -135,6 +135,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, return property; fail: kfree(property->values); +free_property: kfree(property); return NULL; }