On Tue, Oct 29, 2024 at 12:28:20PM +0100, Stefano Brivio wrote: 61;7604;1c> In pcap_init(), we should always open the packet capture file withO_CLOEXEC, even if we're not running in foreground: O_CLOEXEC means close-on-exec, not close-on-fork. In logfile_init() and pidfile_open(), the fact that we pass a third 'mode' argument to open() seems to confuse the android-cloexec-open checker in LLVM versions from 16 to 19 (at least). The checker is suggesting to add O_CLOEXEC to 'mode', and not in 'flags', where we already have it. Add a suppression for clang-tidy and a comment, and avoid repeating those three times by adding a new helper, output_file_open(). Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>--- conf.c | 5 ++++- log.c | 3 +-- pcap.c | 7 ++----- util.c | 27 +++++++++++---------------- util.h | 2 +- 5 files changed, 19 insertions(+), 25 deletions(-) diff --git a/conf.c b/conf.c index 4db7c64..d6faa5e 100644 --- a/conf.c +++ b/conf.c @@ -1194,7 +1194,10 @@ static void conf_open_files(struct ctx *c) if (c->mode != MODE_PASTA && c->fd_tap == -1) c->fd_tap_listen = tap_sock_unix_open(c->sock_path); - c->pidfile_fd = pidfile_open(c->pidfile); + if (*c->pidfile) { + if ((c->pidfile_fd = output_file_open(c->pidfile, 0)) < 0) + die_perror("Couldn't open PID file %s", c->pidfile); + } } /** diff --git a/log.c b/log.c index 6932885..0adddff 100644 --- a/log.c +++ b/log.c @@ -416,8 +416,7 @@ void logfile_init(const char *name, const char *path, size_t size) if (readlink("/proc/self/exe", exe, PATH_MAX - 1) < 0) die_perror("Failed to read own /proc/self/exe link"); - log_file = open(path, O_CREAT | O_TRUNC | O_APPEND | O_RDWR | O_CLOEXEC, - S_IRUSR | S_IWUSR); + log_file = output_file_open(path, O_APPEND); if (log_file == -1) die_perror("Couldn't open log file %s", path); diff --git a/pcap.c b/pcap.c index 6ee6cdf..12737d8 100644 --- a/pcap.c +++ b/pcap.c @@ -158,18 +158,15 @@ void pcap_iov(const struct iovec *iov, size_t iovcnt, size_t offset) */ void pcap_init(struct ctx *c) { - int flags = O_WRONLY | O_CREAT | O_TRUNC; - if (pcap_fd != -1) return; if (!*c->pcap) return; - flags |= c->foreground ? O_CLOEXEC : 0; - pcap_fd = open(c->pcap, flags, S_IRUSR | S_IWUSR); + pcap_fd = output_file_open(c->pcap, 0); if (pcap_fd == -1) { - perror("open"); + err_perror("Couldn't open pcap file %s", c->pcap); return; } diff --git a/util.c b/util.c index 9cb705e..1ad3b5c 100644 --- a/util.c +++ b/util.c @@ -407,25 +407,20 @@ void pidfile_write(int fd, pid_t pid) } /** - * pidfile_open() - Open PID file if needed - * @path: Path for PID file, empty string if no PID file is requested + * output_file_open() - Open file for output, if needed + * @path: Path for output file + * @flags: Additional flags for open() * - * Return: descriptor for PID file, -1 if path is NULL, won't return on failure + * Return: file descriptor on success, -1 on failure with errno set by open() */ -int pidfile_open(const char *path) +int output_file_open(const char *path, int flags) { - int fd; - - if (!*path) - return -1; - - if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC, - S_IRUSR | S_IWUSR)) < 0) { - perror("PID file open"); - exit(EXIT_FAILURE); - } - - return fd; + /* We use O_CLOEXEC here, but clang-tidy as of LLVM 16 to 19 looks for + * it in the 'mode' argument if we have one + */ + return open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC | flags, + /* NOLINTNEXTLINE(android-cloexec-open) */ + S_IRUSR | S_IWUSR); } /** diff --git a/util.h b/util.h index 4f8b768..3fc64cf 100644 --- a/util.h +++ b/util.h @@ -193,7 +193,7 @@ char *line_read(char *buf, size_t len, int fd); void ns_enter(const struct ctx *c); bool ns_is_init(void); int open_in_ns(const struct ctx *c, const char *path, int flags); -int pidfile_open(const char *path); +int output_file_open(const char *path, int flags); void pidfile_write(int fd, pid_t pid); int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x);-- 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