[PATCH] fs: fuse: check return value of fuse_simple_request

[PATCH] fs: fuse: check return value of fuse_simple_request

[Cengiz Can]

In `fs/fuse/file.c` `fuse_simple_request` is used in multiple places,
with its return value properly checked for possible errors.

However the usage on `fuse_file_put` ignores its return value. And the
following `fuse_release_end` call used hard-coded error value of `0`.

This triggers a warning in static analyzers and such.

I've added a variable to capture `fuse_simple_request` result and passed
that to `fuse_release_end` instead.

Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
---
 fs/fuse/file.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index a63d779eac10..9914ee2af311 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -110,6 +110,7 @@ static void fuse_release_end(struct fuse_conn *fc, struct fuse_args *args,

 static void fuse_file_put(struct fuse_file *ff, bool sync, bool isdir)
 {
+	int err;
 	if (refcount_dec_and_test(&ff->count)) {
 		struct fuse_args *args = &ff->release_args->args;

@@ -117,8 +118,8 @@ static void fuse_file_put(struct fuse_file *ff, bool sync, bool isdir)
 			/* Do nothing when client does not implement 'open' */
 			fuse_release_end(ff->fc, args, 0);
 		} else if (sync) {
-			fuse_simple_request(ff->fc, args);
-			fuse_release_end(ff->fc, args, 0);
+			err = fuse_simple_request(ff->fc, args);
+			fuse_release_end(ff->fc, args, err);
 		} else {
 			args->end = fuse_release_end;
 			if (fuse_simple_background(ff->fc, args,
--
2.25.0

Re: [PATCH] fs: fuse: check return value of fuse_simple_request

[Miklos Szeredi]

On Mon, Jan 20, 2020 at 1:13 PM Cengiz Can <cengiz@kernel.wtf> wrote:
>
> In `fs/fuse/file.c` `fuse_simple_request` is used in multiple places,
> with its return value properly checked for possible errors.
>
> However the usage on `fuse_file_put` ignores its return value. And the
> following `fuse_release_end` call used hard-coded error value of `0`.
>
> This triggers a warning in static analyzers and such.
>
> I've added a variable to capture `fuse_simple_request` result and passed
> that to `fuse_release_end` instead.

Which then goes on to ignore the error, so we are exactly where we
were with some added obscurity, which will be noticed by the next
generation of static analyzer, when you'd come up with an even more
obscure way to ignore the error, etc...  This leads to nowhere.

If this matters (not sure) then we'll need a notation to ignore the
return value.  Does casting to (void) work?

Thanks,
Miklos

Re: [PATCH] fs: fuse: check return value of fuse_simple_request

[Cengiz Can]

On 2020-01-20 16:39, Miklos Szeredi wrote:
> On Mon, Jan 20, 2020 at 1:13 PM Cengiz Can <cengiz@kernel.wtf> wrote:
>> 
>> In `fs/fuse/file.c` `fuse_simple_request` is used in multiple places,
>> with its return value properly checked for possible errors.
>> 
>> However the usage on `fuse_file_put` ignores its return value. And the
>> following `fuse_release_end` call used hard-coded error value of `0`.
>> 
>> This triggers a warning in static analyzers and such.
>> 
>> I've added a variable to capture `fuse_simple_request` result and 
>> passed
>> that to `fuse_release_end` instead.
> 
> Which then goes on to ignore the error, so we are exactly where we
> were with some added obscurity, which will be noticed by the next
> generation of static analyzer, when you'd come up with an even more
> obscure way to ignore the error, etc...  This leads to nowhere.

I got your point. Thanks for explaining.

> If this matters (not sure) then we'll need a notation to ignore the
> return value.  Does casting to (void) work?

It should probably work for the sake of silencing the analyzer but I 
think
it would be easier to just ignore the warning and mark is as 
unimportant.

IMHO code should be as readable as possible. So not point in casting it.

If `fuse_simple_request` errors are very rare, we can ignore this patch.

Thank you

> 
> Thanks,
> Miklos

-- 
Cengiz Can
@cengiz_io