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

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:

quoted

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