ext4 crypto: testing the wrong variable
Subsystems (2)
PATCH 1/1 ext4 crypto: testing the wrong variable
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 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.
fs/ext4/crypto_fname.c +1 -1
@@ -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 ;
fs/ext4/crypto_fname.c int ext4_fname_crypto_alloc_buffer(struct ext4_fname_crypto_ctx *ctx, · line 561
Quoted 4 lines
void
ext4_fname_crypto_free_buffer(void **buf)
{fs/ext4/crypto_fname.c +1
-1-if (*buf =NULL ||IS_ERR (buf)) +if (*buf =NULL ||IS_ERR (*buf))
Why not use IS_ERR_OR_NULL?
julia
Quoted 8 lines
fs/ext4/crypto_fname.c +0
-1return ;kfree (*buf); *buf =NULL ; --To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 08.04.2015 11:51, schrieb Julia Lawall:
Quoted 8 lines
fs/ext4/crypto_fname.c int ext4_fname_crypto_alloc_buffer(struct ext4_fname_crypto_ctx *ctx, · line 561
Quoted 4 lines
void
ext4_fname_crypto_free_buffer(void **buf)
{fs/ext4/crypto_fname.c +1
-1-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 ?
Quoted 14 lines
Quoted 9 lines
fs/ext4/crypto_fname.c +0
-1return ;kfree (*buf); *buf =NULL ; --To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 13 lines · walter harms
Am 08.04.2015 11:51, schrieb Julia Lawall:
Quoted 8 lines
fs/ext4/crypto_fname.c int ext4_fname_crypto_alloc_buffer(struct ext4_fname_crypto_ctx *ctx, · line 561
Quoted 4 lines
void
ext4_fname_crypto_free_buffer(void **buf)
{fs/ext4/crypto_fname.c +1
-1-if (*buf =NULL ||IS_ERR (buf)) +if (*buf =NULL ||IS_ERR (*buf))Why not use
IS_ERR_OR_NULL?julia
Quoted 6 lines
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
Theodore Ts'o
maintainerQuoted 9 lines · Dan Carpenter
Quoted 6 lines
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
Quoted 2 lines · Theodore Ts'o
(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
Quoted 4 lines · Dan Carpenter
The intention was to check "
IS_ERR(*buf)" instead of "IS_ERR(buf)". It's never anERR_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
Quoted 20 lines
Signed-off-by Dan Carpenter <[email protected]>---
I have a static checker warning for when people checkIS_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_PTRtoext4_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.fs/ext4/crypto_fname.c +0
-0
fs/ext4/crypto_fname.c int ext4_fname_crypto_alloc_buffer(struct ext4_fname_crypto_ctx *ctx, · line 564
Quoted 8 lines
fs/ext4/crypto_fname.c +1
-1@@ -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