On Fri, Oct 25, 2024 at 01:04:38AM +0200, Stefano Brivio wrote:
> I thought we could just set errno to 0, do a bunch of stuff, and check
> that errno didn't change to infer we succeeded. But clang-tidy,
> starting with LLVM 19, reports:
> 
> /home/sbrivio/passt/util.c:465:6: error: An undefined value may be read from 'errno' [clang-analyzer-unix.Errno,-warnings-as-errors]
>   465 |         if (errno)
>       |             ^
> /usr/include/errno.h:38:16: note: expanded from macro 'errno'
>    38 | # define errno (*__errno_location ())
>       |                ^~~~~~~~~~~~~~~~~~~~~~
> /home/sbrivio/passt/util.c:446:6: note: Assuming the condition is false
>   446 |         if (pid == -1) {
>       |             ^~~~~~~~~
> /home/sbrivio/passt/util.c:446:2: note: Taking false branch
>   446 |         if (pid == -1) {
>       |         ^
> /home/sbrivio/passt/util.c:451:6: note: Assuming 'pid' is 0
>   451 |         if (pid) {
>       |             ^~~
> /home/sbrivio/passt/util.c:451:2: note: Taking false branch
>   451 |         if (pid) {
>       |         ^
> /home/sbrivio/passt/util.c:463:2: note: Assuming that 'close' is successful; 'errno' becomes undefined after the call
>   463 |         close(devnull_fd);
>       |         ^~~~~~~~~~~~~~~~~
> /home/sbrivio/passt/util.c:465:6: note: An undefined value may be read from 'errno'
>   465 |         if (errno)
>       |             ^
> /usr/include/errno.h:38:16: note: expanded from macro 'errno'
>    38 | # define errno (*__errno_location ())
>       |                ^~~~~~~~~~~~~~~~~~~~~~
> 
> And the LLVM documentation for the unix.Errno checker, 1.1.8.3
> unix.Errno (C), mentions, at:
> 
>   https://clang.llvm.org/docs/analyzer/checkers.html#unix-errno
> 
> that:
> 
>   The C and POSIX standards often do not define if a standard library
>   function may change value of errno if the call does not fail.
>   Therefore, errno should only be used if it is known from the return
>   value of a function that the call has failed.
> 
> which is, somewhat surprisingly, the case for close().

Ah, yeah.

> Instead of using errno, check the actual return values of the calls
> we issue here.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  util.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/util.c b/util.c
> index b719f9a..02e18fb 100644
> --- a/util.c
> +++ b/util.c
> @@ -453,16 +453,11 @@ int __daemon(int pidfile_fd, int devnull_fd)
>  		exit(EXIT_SUCCESS);
>  	}
>  
> -	errno = 0;
> -
> -	setsid();
> -
> -	dup2(devnull_fd, STDIN_FILENO);
> -	dup2(devnull_fd, STDOUT_FILENO);
> -	dup2(devnull_fd, STDERR_FILENO);
> -	close(devnull_fd);
> -
> -	if (errno)
> +	if (setsid()				< 0 ||
> +	    dup2(devnull_fd, STDIN_FILENO)	< 0 ||
> +	    dup2(devnull_fd, STDOUT_FILENO)	< 0 ||
> +	    dup2(devnull_fd, STDERR_FILENO)	< 0 ||
> +	    close(devnull_fd))
>  		exit(EXIT_FAILURE);
>  
>  	return 0;

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson