On Saturday 2021-10-30 18:44, Jeremy Sowden wrote:
>If the path is already bound, we close the socket immediately.
>diff --git a/input/packet/ulogd_inppkt_UNIXSOCK.c b/input/packet/ulogd_inppkt_UNIXSOCK.c
>index f97c2e174b2d..d88609f203c4 100644
>--- a/input/packet/ulogd_inppkt_UNIXSOCK.c
>+++ b/input/packet/ulogd_inppkt_UNIXSOCK.c
>@@ -479,10 +479,17 @@ static int _create_unix_socket(const char *unix_path)
> int s;
> struct stat st_dummy;
>
>+ if (stat(unix_path, &st_dummy) == 0 && st_dummy.st_size > 0) {
>+ ulogd_log(ULOGD_ERROR,
>+ "ulogd2: unix socket '%s' already exists\n",
>+ unix_path);
>+ return -1;
>+ }
>+
That stat call should just be entirely deleted.
I fully expect that Coverity's static analyzer (or something like it)
is going to flag this piece of code as running afoul of TOCTOU.
A foreign event may cause the socket to come into existence between stat() and
bind(), and therefore the code needs to handle the bind(2) failure _in any
case_, and can report "Oh but it exists" at that time.
So even if the stat call is fine from a security POV, it is redundant code.
