Re: analyzer: implement five new warnings for misuse of POSIX

Re: analyzer: implement five new warnings for misuse of POSIX

[Jay K]

 > check for double "close" of a FD (CWE-1341).
 > check for read/write of a closed file descriptor 
 
 These sound good but kinda non general or incomplete to me. 
 I mean, isn't the "right" thing, to disallow passing 
 a closed fd to "almost any" function?  

 But I realize "almost any" is difficult to pin down. 
  fd = open(); 
  close(fd); 
  printf("%d", fd);  

is often ok (assuming nobody reads the output, string to int,
back to close/read/write). It is any path leading to,
a long list, like close, read, write, ioctl, send, recv, etc.
and I don't know if "path leading to" is possible to model here, haven't looked, sorry.

 - Jay

Re: analyzer: implement five new warnings for misuse of POSIX

[David Malcolm]

On Sun, 2022-07-03 at 02:46 +0000, Jay K wrote:
>  > check for double "close" of a FD (CWE-1341).
>  > check for read/write of a closed file descriptor 
>  
>  These sound good but kinda non general or incomplete to me. 
>  I mean, isn't the "right" thing, to disallow passing 
>  a closed fd to "almost any" function?  
> 
>  But I realize "almost any" is difficult to pin down. 
>   fd = open(); 
>   close(fd); 
>   printf("%d", fd);  
> 
> is often ok (assuming nobody reads the output, string to int,
> back to close/read/write). It is any path leading to,
> a long list, like close, read, write, ioctl, send, recv, etc.
> and I don't know if "path leading to" is possible to model here,
> haven't looked, sorry.

Jay: right now, the only function calls that these FD warnings know
about are open, close, read, and write: the only "use" that -Wanalyzer-
fd-use-after-close currently cares about is "read" and "write" (and
double-close is its own diagnostic).

I put together an example on Compiler Explorer here which tries to show
as many of the new warnings as possible:
  https://godbolt.org/z/rsc51PYoK  [1]

I think we're going to want to add a new function attribute that
signifies
  "this 'int' parameter is expected to be an open file descriptor"
so that e.g. glibc's headers can use the attribute to mark up all the
places expecting such FDs, and thus -fanalyzer can warn about misuses,
lacking checks, etc.

Probably some sort of support for dup, dup2, etc.


Immad: we might want some test coverage that Jay's example doesn't
generate a warning from -fanalyzer.


Hope this makes sense
Dave

[1] I also put in a bounds-check bug in the first "write" call, which
sadly we don't complain about yet.  I've added a note about that to 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106000