On 3/28/23 17:55, Markus Elfring wrote:
> Hello,
>
> I tried the following SmPL script out also on the source files from
> the software “Linux next-20230324”.
>
> @display@
> expression call, storage;
> identifier target;
> @@
> *storage = call(...);
> *if (!storage)
> goto target;
> ...
> *target:
> *kfree(storage)
>
>
> 119 source files were found where a check was performed for a failed function call
> and a kfree() call is immediately performed for the same variable.
> Thus a null pointer is probably passed at these source code places.
It is valid to pass a NULL pointer to kfree(), it checks for that.
> I imagine that such code should be reconsidered once more and improved accordingly.
The only potential downside in the scenario is checking storage == NULL
twice. But this is an error handling path, not a fast path. On the other
hand, the code may look like this:
storage = call(...);
if (!storage)
goto target;
if (some_other_condition)
goto target;
target:
kfree(storage)
For some_other_condition, we want the kfree(). If you changed the code, to
remove the extra NULL pointer check, you would have:
storage = call(...);
if (!storage)
goto target1;
if (some_other_condition)
goto target2;
target2:
kfree(storage)
target1:
The extra goto label and larger code is not worth saving the NULL pointer
check, because this is error handling path.
> How do you think about to achieve any adjustments in this design direction?
Only in cases where it would not make the code more complex, i.e. if there
are no "some_other_condition" that jumps to the same target after allocation
to storage is successful.
Vlastimil
> Regards,
> Markus
