[patch] ext4 crypto: testing the wrong variable

[patch] ext4 crypto: testing the wrong variable

[Dan Carpenter]

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.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
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;

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

[Julia Lawall]

>  void ext4_fname_crypto_free_buffer(void **buf)
>  {
> -	if (*buf == NULL || IS_ERR(buf))
> +	if (*buf == NULL || IS_ERR(*buf))

Why not use IS_ERR_OR_NULL?

julia

>  		return;
>  	kfree(*buf);
>  	*buf = NULL;
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[walter harms]

Am 08.04.2015 11:51, schrieb Julia Lawall:
>>  void ext4_fname_crypto_free_buffer(void **buf)
>>  {
>> -	if (*buf == NULL || IS_ERR(buf))
>> +	if (*buf == NULL || IS_ERR(*buf))
> 
> Why not use IS_ERR_OR_NULL?
> 
> julia


why test *buf == NULL ? xfree() can handle this.

the question is do programm depend on *buf=NULL.
In case of IS_ERR(*buf) *buf will be left unchanged
and later prgramms may things there is a buffer
available ?

> 
>>  		return;
>>  	kfree(*buf);
>>  	*buf = NULL;
>> --
>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Dan Carpenter]

On Wed, Apr 08, 2015 at 01:51:08PM +0200, walter harms wrote:
> 
> 
> Am 08.04.2015 11:51, schrieb Julia Lawall:
> >>  void ext4_fname_crypto_free_buffer(void **buf)
> >>  {
> >> -	if (*buf == NULL || IS_ERR(buf))
> >> +	if (*buf == NULL || IS_ERR(*buf))
> > 
> > Why not use IS_ERR_OR_NULL?
> > 
> > julia
> 
> 
> why test *buf == NULL ? xfree() can handle this.
> 
> the question is do programm depend on *buf=NULL.
> In case of IS_ERR(*buf) *buf will be left unchanged
> and later prgramms may things there is a buffer
> available ?

Good point.  That IS_ERR() check is going to cause all kinds of future
bugs.

regards,
dan carpenter

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

[Theodore Ts'o]

On Wed, Apr 08, 2015 at 03:22:51PM +0300, Dan Carpenter wrote:
> > why test *buf == NULL ? xfree() can handle this.
> > 
> > the question is do programm depend on *buf=NULL.
> > In case of IS_ERR(*buf) *buf will be left unchanged
> > and later prgramms may things there is a buffer
> > available ?
> 
> Good point.  That IS_ERR() check is going to cause all kinds of future
> bugs.

Yes, it's not needed at all, so I'll just remove it.  I'm also going
to be fixing the whole fname_crypto_alloc_buffer() and
fname_crypto_free_buffer() so the calling convention isn't as
horrendous.  So basically, instead of

int ext4_fname_crypto_alloc_buffer(struct ext4_fname_crypto_ctx *ctx,
				   unsigned char **obuf, u32 *olen, u32 ilen);
void ext4_fname_crypto_free_buffer(void **buf);

it will be:

int ext4_fname_crypto_alloc_buffer(struct ext4_fname_crypto_ctx *ctx,
				   struct ext4_str *crypto_str, u32 ilen);
void ext4_fname_crypto_free_buffer(struct ext4_str *crypto_str);

(And BTW, this isn't appropriate for kernel-janitors since this code
isn't upstream yet)

							- Ted

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

[Dan Carpenter]

On Sat, Apr 11, 2015 at 09:48:11AM -0400, Theodore Ts'o wrote:
> (And BTW, this isn't appropriate for kernel-janitors since this code
> isn't upstream yet)

Julia and I keep tabs on each other through kernel janitors.  It used to
be more of an issue when Vasiliy Kulikov was around because we were
always sending duplicate fixes and maintainers were getting annoyed with
us.  I miss Vasiliy, he's off doing open wall stuff now.

regards,
dan carpenter

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

[Andreas Dilger]

On Apr 8, 2015, at 2:53 AM, Dan Carpenter <dan.carpenter@oracle.com> 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@oracle.com>
> ---
> 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