Re: [PATCH 1/1] 9p-local.c: always return -1 on error in local_unlinkat_common

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 1/7/20 1:47 PM, Greg Kurz wrote:
> I've changed "9p-local.c:" to "9p: local:" which is the usual prefix used for
> fsdev backend specific changes.

Good to know. Thanks for fixing it up.

> 
> On Tue,  7 Jan 2020 11:47:18 -0300
> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> 
>> local_unlinkat_common() is supposed to always return -1 on error.
>> This is being done by jumps to the 'err_out' label, which is
>> a 'return ret' call, and 'ret' is initialized with -1.
>>
>> Unfortunately there is a condition in which the function will
>> return 0 on error: in a case where flags == AT_REMOVEDIR, 'ret'
>> will be 0 when reaching
>>
>> map_dirfd = openat_dir(...)
>>
>> And, if map_dirfd == -1 and errno != ENOENT, the existing 'err_out'
>> jump will execute 'return ret', when ret is still set to zero
>> at that point.
>>
>> This patch fixes it by changing all 'err_out' labels by
>> 'return -1' calls, ensuring that the function will always
>> return -1 on error conditions. 'ret' can be left unintialized
>> since it's now being used just to store the result of 'unlinkat'
>> calls.
>>
>> CC: Greg Kurz <groug@kaod.org>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
> 
> Applied to 9p-next.
> 
> Thanks Daniel and feliz ano novo :)


Bonne année!




> 
>>   hw/9pfs/9p-local.c | 14 ++++++--------
>>   1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
>> index ca641390fb..de61aca216 100644
>> --- a/hw/9pfs/9p-local.c
>> +++ b/hw/9pfs/9p-local.c
>> @@ -1076,7 +1076,7 @@ out:
>>   static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name,
>>                                    int flags)
>>   {
>> -    int ret = -1;
>> +    int ret;
>>   
>>       if (ctx->export_flags & V9FS_SM_MAPPED_FILE) {
>>           int map_dirfd;
>> @@ -1094,12 +1094,12 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name,
>>   
>>               fd = openat_dir(dirfd, name);
>>               if (fd == -1) {
>> -                goto err_out;
>> +                return -1;
>>               }
>>               ret = unlinkat(fd, VIRTFS_META_DIR, AT_REMOVEDIR);
>>               close_preserve_errno(fd);
>>               if (ret < 0 && errno != ENOENT) {
>> -                goto err_out;
>> +                return -1;
>>               }
>>           }
>>           map_dirfd = openat_dir(dirfd, VIRTFS_META_DIR);
>> @@ -1107,16 +1107,14 @@ static int local_unlinkat_common(FsContext *ctx, int dirfd, const char *name,
>>               ret = unlinkat(map_dirfd, name, 0);
>>               close_preserve_errno(map_dirfd);
>>               if (ret < 0 && errno != ENOENT) {
>> -                goto err_out;
>> +                return -1;
>>               }
>>           } else if (errno != ENOENT) {
>> -            goto err_out;
>> +            return -1;
>>           }
>>       }
>>   
>> -    ret = unlinkat(dirfd, name, flags);
>> -err_out:
>> -    return ret;
>> +    return unlinkat(dirfd, name, flags);
>>   }
>>   
>>   static int local_remove(FsContext *ctx, const char *path)
>