fileno() Does Not Check for NULL

fileno() Does Not Check for NULL

[Joel Sherrill]

Hi

In looking at Coverity issues, we are seeing that calls to fileno() are
getting flagged as potential NULL dereferences. Looking at the POSIX
definition, it seems that it should return an -1/EBADF because NULL isn't a
valid stream.

https://pubs.opengroup.org/onlinepubs/9699919799/functions/fileno.html

I know there is a pattern of not having NULL checks assuming that the
environment would catch the fault. But in the embedded environments newlib
is used in, there isn't anything to catch the fault.

I don't want to add application code to check for a NULL before calling a
standard method that isn't robustly checking its arguments.

I don't know if it would address the Coverity issue but would adding the
nonnull attribute on more methods be acceptable and help? It is defined and
used in a few places now.

What's the right approach to addressing this?

Thanks.

--joel

Re: fileno() Does Not Check for NULL

[Eric Blake]

On 2/17/21 9:57 AM, Joel Sherrill wrote:
> Hi
> 
> In looking at Coverity issues, we are seeing that calls to fileno() are
> getting flagged as potential NULL dereferences. Looking at the POSIX
> definition, it seems that it should return an -1/EBADF because NULL isn't a
> valid stream.
> 
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/fileno.html

Wrong.  In general, POSIX says that foo(NULL) is undefined behavior
unless foo() specifically documents what the function must do with a
null pointer.  The EBADF error mentioned in fileno() is for cases like
open_memstream() which gives you a FILE* with no underlying fd, and not
for cases where you pass something like NULL that is not even a FILE*.
Or put another way, POSIX says that fileno(NULL) is undefined behavior
(it might crash, it might fail with EFAULT, it might be a nop, it might
reformat your hard drive...), and therefore the bug is in your code for
attempting it, and not in fileno() for not diagnosing it.

> 
> I know there is a pattern of not having NULL checks assuming that the
> environment would catch the fault. But in the embedded environments newlib
> is used in, there isn't anything to catch the fault.

When you trigger undefined behavior, the bug is yours, not newlib's.

> 
> I don't want to add application code to check for a NULL before calling a
> standard method that isn't robustly checking its arguments.

It's not newlib's job to work around your code's lack of robustness.

> 
> I don't know if it would address the Coverity issue but would adding the
> nonnull attribute on more methods be acceptable and help? It is defined and
> used in a few places now.

A patch adding the nonnull attribute to declarations such as filen() to
let the compiler help you diagnose your buggy code is probably
acceptable (it's easier to justify if other libcs, like glibc, do the
same; but my quick check shows that glibc has not marked fileno() with a
nonnull attribute).  But adding an if(NULL) check to the implementation
of fileno() is not.

> 
> What's the right approach to addressing this?
> 
> Thanks.
> 
> --joel
> -- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org