Re: [PATCH V2] Fix return value from fstat calls

["Tkaczyk, Mariusz" <mariusz.tkaczyk@linux.intel.com>]

On 13.08.2021 01:23, NeilBrown wrote:
>>
>> The fstat() function will fail if:
>> [EBADF] - The fildes argument is not a valid file descriptor.
> 
> But we never pass an invalid file descriptor
> 
We can't guarantee that. There is always a minimal chance to pass
wrong/invalid argument caused by bug somewhere else in mdadm logic.
I think that handling such case is reasonable from security point
of view but agree that it could be a dead check (if everything is
well implemented).

> And you didn't list "EFAULT", but of course we never pass an invalid
> memory address either.

As before, we can't guarantee it, too. For now it seems to be well handled
but implementation may change and vulnerability might be missed during
review. Is safer to handle that some way.

>> [EIO] - An I/O error occurred while reading from the file system.
> 
> fstat() doesn't do IO, it just reports data from the cache.
> 
>> [EOVERFLOW] - The file size in bytes or the number of blocks allocated to the file or the file serial number cannot be represented correctly in the structure pointed to by buf.
>>
>> The fstat() function may fail if:
>>
>> [EOVERFLOW] - One of the values is too large to store into the structure pointed to by the buf argument.
>>
> 
> Those don't happen in practice for the fstat() calls that mdadm makes
> either.
> 
Agree, but it could be changed.

> I think this patch is adding noise to the source code without actually
> providing any real value.  I would much prefer that if you really feel
> there is value, then just add a wrapper:
> 
> int  safe_fstat(....)
> {
>      int ret = fstat(.....);
>      char message[]="mdadm: fstat failed, so aborting\n"
>      if (ret == 0)
>           return 0;
>      write(2, message, sizeof(message)-1);
>      exit(1);
> }
> 
> Then just change every "fstat" in the code that bothers you to
> "safe_fstat()".
> 
> This approach of adding pointless checks because some static analysis
> tool thinks you should is not an approach that I approve of.

Maybe It won't be useful for users but it may help developers to avoid
trivial mistakes. As you told, if everything is fine then check is dead.
In my opinion any error handling is better than nothing.

Anyway, I think that there is a lot of more dangerous lines.

Regards,
Mariusz