Re: [PATCH] util, passt: Close daemon-lifetime fds on exit to avoid Coverity warning

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4350 bytes --]

On Sun, May 31, 2026 at 04:13:24PM -0400, Jon Maloy wrote:
> conf_open_files() opens four file descriptors (fd_tap_listen,
> fd_repair_listen, pidfile_fd, fd_control_listen) that are held for
> the entire daemon lifetime.  Because no close() call exists for them
> anywhere, Coverity flags each as INCOMPLETE_DEALLOCATOR. This is
> clearly a false positive, but we still want to get rid of this
> warning.

A bit frustrating that Coverity doesn't recognize an exit() will close
all remaining files, but ok.  I'm ok with close()ing these at
exit... but some details remain.

First, for pidfile_fd specifically, we shouldn't need that after we've
written the pid.  So we should just close() at that point, rather than
waiting until exit.

> 
> We now register the execution context so that passt_exit() can use
> to close these descriptors before calling _exit().  All exit paths
> (signal handler, die(), die_perror()) funnel through passt_exit(),
> so this covers all cases.

passt_exit() is the right place for this, but registering an actual
global to point at a local that's sort of a global seems pretty
perverse.  It would make more sense to convert the main struct ctx
itself to a global, rather than a main()-local.  Note that I'm not
suggesting changing all the places that use it (current or future) - I
think it's still preferable for those (excepting I guess main()
itself) to access it via a parameter.  We can encourage that by using
a bulky name for the global.

I see that setting 'exit_cleanup_ctx' where you do also serves one
other purpose - it stops early passt_exit()s from attempting to
close() those fds before they're initialised, even to -1.  We can fix
that by using an initialiser on the global to set fd fields to -1
before main(), which is arguably more elegant than the cluster of
assignments at the top of main() anyway.


> 
> Signed-off-by: Jon Maloy <jmaloy@redhat.com>
> ---
>  log.h   |  2 ++
>  passt.c |  1 +
>  util.c  | 22 ++++++++++++++++++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/log.h b/log.h
> index 69cfb507..079f429c 100644
> --- a/log.h
> +++ b/log.h
> @@ -63,7 +63,9 @@ extern bool debug_flag;
>  /* This would make more sense in util.h, but because we use it in die(), that
>   * would cause awkward circular reference problems.
>   */
> +struct ctx;
>  void passt_exit(int status) __attribute__((noreturn));
> +void passt_exit_set_ctx(struct ctx *c);
>  
>  #define LOGFILE_SIZE_DEFAULT		(1024 * 1024UL)
>  #define LOGFILE_CUT_RATIO		30	/* When full, cut ~30% size */
> diff --git a/passt.c b/passt.c
> index b6fc12d4..ec6aa57a 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -392,6 +392,7 @@ int main(int argc, char **argv)
>  	sock_probe_features(&c);
>  
>  	conf(&c, argc, argv);
> +	passt_exit_set_ctx(&c);
>  	trace_init(c.trace);
>  
>  	pasta_netns_quit_init(&c);
> diff --git a/util.c b/util.c
> index b64c29ed..f15b1f9a 100644
> --- a/util.c
> +++ b/util.c
> @@ -1097,6 +1097,17 @@ void abort_with_msg(const char *fmt, ...)
>  	abort();
>  }
>  
> +static struct ctx *exit_cleanup_ctx;
> +
> +/**
> + * passt_exit_set_ctx() - Register context for cleanup on exit
> + * @c:		Execution context
> + */
> +void passt_exit_set_ctx(struct ctx *c)
> +{
> +	exit_cleanup_ctx = c;
> +}
> +
>  /**
>   * passt_exit() - Perform vital cleanup and exit
>   *
> @@ -1108,6 +1119,17 @@ void abort_with_msg(const char *fmt, ...)
>   */
>  void passt_exit(int status)
>  {
> +	if (exit_cleanup_ctx) {
> +		if (exit_cleanup_ctx->fd_tap_listen >= 0)
> +			close(exit_cleanup_ctx->fd_tap_listen);
> +		if (exit_cleanup_ctx->fd_repair_listen >= 0)
> +			close(exit_cleanup_ctx->fd_repair_listen);
> +		if (exit_cleanup_ctx->pidfile_fd >= 0)
> +			close(exit_cleanup_ctx->pidfile_fd);
> +		if (exit_cleanup_ctx->fd_control_listen >= 0)
> +			close(exit_cleanup_ctx->fd_control_listen);
> +	}
> +
>  	/* Make sure we don't leave the pcap file truncated */
>  	if (pcap_fd != -1 && fsync(pcap_fd))
>  		warn_perror("Failed to flush pcap file, it might be truncated");
> -- 
> 2.52.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]