Ally Heev wrote:
> uninitialized pointers with __free attribute can cause undefined
> behaviour as the memory allocated to the pointer is freed
> automatically when the pointer goes out of scope.
> add check in checkpatch to detect such issues
>
> Suggested-by: Dan Carpenter <[email protected]>
> Link:
> https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Ally Heev <[email protected]>
> ---
> Test:
> ran checkpatch.pl before and after the change on
> crypto/asymmetric_keys/x509_public_key.c, which has
> both initialized and uninitialized pointers
> ---
> Documentation/dev-tools/checkpatch.rst | 5 +++++
> scripts/checkpatch.pl | 6 ++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst
> b/Documentation/dev-tools/checkpatch.rst
> index
> d5c47e560324fb2399a5b1bc99c891ed1de10535..1a304bf38bcd27e50bbb7cd4383b07ac54d20b0a
> 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -1009,6 +1009,11 @@ Functions and Variables
>
> return bar;
>
> + **UNINITIALIZED_PTR_WITH_FREE**
> + Pointers with __free attribute should be initialized. Not doing so
> + may lead to undefined behavior as the memory allocated (garbage,
> + in case not initialized) to the pointer is freed automatically
> + when the pointer goes out of scope.
>
> Permissions
> -----------
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index
> 92669904eecc7a8d2afd3f2625528e02b6d17cd6..33cb09843431bebef72a4f5daab3a5d321bcb911
> 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -7721,6 +7721,12 @@ sub process {
> ERROR("MISSING_SENTINEL", "missing sentinel in
> ID array\n" . "$here\n$stat\n");
> }
> }
> +
> +# check for uninitialized pointers with __free attribute
> + if ($line =~
> /\s*$Type\s*($Ident)\s+__free\s*\(\s*$Ident\s*\)\s*;/) {
> + WARN("UNINITIALIZED_PTR_WITH_FREE",
> + "pointer '$1' with __free attribute should be
> initialized\n" . $herecurr);
Looks good to me, but I why WARN and not ERROR? Is there ever a valid
reason to ignore this warning?
I would go futher and suggest that the pattern of:
type foo __free(free_foo) = NULL;
...be made into a warning because that easily leads to situations where
declaration order is out of sync with allocation order. I.e. can be made
technically correct, but at a level of cleverness that undermines the
benefit.
With or without the conversion to ERROR() for the above,
Acked-by: Dan Williams <[email protected]>
