On Thu, 12 Aug 2021, Nigel Croxon wrote: > On Wednesday, Aug 11, 2021 at 6:53 PM, NeilBrown <neilb@xxxxxxx (mailto:neilb@xxxxxxx)> 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
