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

NeilBrown
2021-08-12T23:23:27Z

On Thu, 12 Aug 2021, Nigel Croxon wrote:
> On Wednesday, Aug 11, 2021 at 6:53 PM, NeilBrown <neilb@suse.de (mailto:neilb@suse.de)> wrote:
> On Thu, 12 Aug 2021, Nigel Croxon wrote:
> > To meet requirements of Common Criteria certification vulnerablility
> > assessment. Static code analysis has been run and found the following
> > errors:
> > check_return: Calling "fstat(fd, &dstb)" without checking return value.
> > This library function may fail and return an error code.
>
> In what circumstances might it fail and return an error code?
>
> NeilBrown
>
> Hello Neil,
>
> The fstat() function will fail if:
> [EBADF] - The fildes argument is not a valid file descriptor.

But we never pass an invalid file descriptor

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

> [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.

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.

But, of course, it is up to Jes what patches he accepts...

NeilBrown