Re: [patch] ext4 crypto: testing the wrong variable

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Apr 8, 2015, at 2:53 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> 
> The intention was to check "IS_ERR(*buf)" instead of "IS_ERR(buf)".
> It's never an ERR_PTR() in the current code so this is a harmless
> mistake.

I was a little confused about this patch, thinking that the ext4 crypto
code had already been merged into master for some reason.  It would
probably have been better as a review comment on the actual patch
"[PATCH 14/22] ext4 crypto: filename encryption facilities" so it isn't
missed when the series is refreshed?

Cheers, Andreas

> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> ---
> I have a static checker warning for when people check IS_ERR() and it's
> not an error pointer.  Adding these extra checks introduces deliberate
> warnings and makes my work harder which means that bugs are missed.
> 
> Also there is no good reason to pass an ERR_PTR to
> ext4_fname_crypto_free_buffer() but eventually someone will take
> advantage of this feature to write ugly code.  Ugly code is going to be
> buggy.
> 
> At least do a "if (WARN_ON(IS_ERR(*buf))) return;".  I could filter out
> impossible conditions which are inside a WARN_ON() and it will mean
> people don't actually pass error pointers here.
> 
> diff --git a/fs/ext4/crypto_fname.c b/fs/ext4/crypto_fname.c
> index 5fda403..f076b52 100644
> --- a/fs/ext4/crypto_fname.c
> +++ b/fs/ext4/crypto_fname.c
> @@ -558,7 +558,7 @@ int ext4_fname_crypto_alloc_buffer(struct ext4_fname_crypto_ctx *ctx,
>  */
> void ext4_fname_crypto_free_buffer(void **buf)
> {
> -	if (*buf == NULL || IS_ERR(buf))
> +	if (*buf == NULL || IS_ERR(*buf))
> 		return;
> 	kfree(*buf);
> 	*buf = NULL;


Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux