From: Markus Elfring <[email protected]>
Date: Tue, 26 Dec 2023 10:26:36 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (3):
One function call less in drm_property_create() after error detection
Delete an unnecessary initialisation in drm_property_create()
Improve four size determinations
drivers/gpu/drm/drm_property.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
--
2.43.0
From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
From: Markus Elfring <[email protected]>
Date: Tue, 26 Dec 2023 08:46:12 +0100
The variable “property” will eventually be set to an appropriate pointer
a bit later. Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/gpu/drm/drm_property.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index 3440f4560e6e..ea365f00e890 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -98,7 +98,7 @@ struct drm_property *drm_property_create(struct drm_device *dev,
u32 flags, const char *name,
int num_values)
{
- struct drm_property *property = NULL;
+ struct drm_property *property;
int ret;
if (WARN_ON(!drm_property_flags_valid(flags)))
--
2.43.0
From: Markus Elfring <[email protected]>
Date: Tue, 26 Dec 2023 09:45:36 +0100
Replace the specification of data structures by pointer dereferences
as the parameter for the operator “sizeof” to make the corresponding size
determination a bit safer according to the Linux coding style convention.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/gpu/drm/drm_property.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c
index ea365f00e890..1fe54cbf1c83 100644
--- a/drivers/gpu/drm/drm_property.c
+++ b/drivers/gpu/drm/drm_property.c
@@ -107,7 +107,7 @@ struct drm_property *drm_property_create(struct drm_device *dev,
if (WARN_ON(strlen(name) >= DRM_PROP_NAME_LEN))
return NULL;
- property = kzalloc(sizeof(struct drm_property), GFP_KERNEL);
+ property = kzalloc(sizeof(*property), GFP_KERNEL);
if (!property)
return NULL;
@@ -418,7 +418,7 @@ int drm_property_add_enum(struct drm_property *property,
if (WARN_ON(index >= property->num_values))
return -EINVAL;
- prop_enum = kzalloc(sizeof(struct drm_property_enum), GFP_KERNEL);
+ prop_enum = kzalloc(sizeof(*prop_enum), GFP_KERNEL);
if (!prop_enum)
return -ENOMEM;
@@ -560,10 +560,10 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
struct drm_property_blob *blob;
int ret;
- if (!length || length > INT_MAX - sizeof(struct drm_property_blob))
+ if (!length || length > INT_MAX - sizeof(*blob))
return ERR_PTR(-EINVAL);
- blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL);
+ blob = kvzalloc(sizeof(*blob) + length, GFP_KERNEL);
if (!blob)
return ERR_PTR(-ENOMEM);
--
2.43.0
The whole series is:
Reviewed-by: Simon Ser <[email protected]>
On 2023-12-26 10:38, Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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?
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
>> 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.
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
>>> 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.
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
> 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