Fix bug when stdin is closed

Fix bug when stdin is closed

[Greg Depoire--Ferrer]

Running iwd with FD 0 (stdin) closed fails because ell considers epoll_fd of 0 to mean failure. The first commit fixes
that bug.

However, it still won't work because ell/dhcp6-transport.c has a bug where it closes a uninitialized FD (FD 0 on my
machine) and that would cause an infinite loop because the epoll_wait call was returning EBADF. The second commit fixes
that bug.

I noticed that ell/dhcp-transport.c is prone to the same mistake as the udp_fd field is not initialized on construction,
but I don't know if the close can actually be called before the udp_fd field is set. Just to be sure, the third commit
initializes it.

[PATCH 1/3] main: Accept FD 0 as epoll_fd

[Greg Depoire--Ferrer]

When create_epoll was called and the file descriptor 0 was not open, the
kernel would return it as the epoll FD, but ell would consider it as an
error because it used the value 0 in epoll_fd to signify failure.

Use -1 instead of 0 to signify error to fix the issue.
---
 ell/main.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/ell/main.c b/ell/main.c
index 1a6cd60..6f9a074 100644
--- a/ell/main.c
+++ b/ell/main.c
@@ -61,7 +61,7 @@
 
 #define WATCHDOG_TRIGGER_FREQ	2
 
-static int epoll_fd;
+static int epoll_fd = -1;
 static bool epoll_running;
 static bool epoll_terminate;
 static int idle_id;
@@ -99,10 +99,8 @@ static inline bool __attribute__ ((always_inline)) create_epoll(void)
 	unsigned int i;
 
 	epoll_fd = epoll_create1(EPOLL_CLOEXEC);
-	if (epoll_fd < 0) {
-		epoll_fd = 0;
+	if (epoll_fd < 0)
 		return false;
-	}
 
 	watch_list = malloc(DEFAULT_WATCH_ENTRIES * sizeof(void *));
 	if (!watch_list)
@@ -121,7 +119,7 @@ static inline bool __attribute__ ((always_inline)) create_epoll(void)
 
 close_epoll:
 	close(epoll_fd);
-	epoll_fd = 0;
+	epoll_fd = -1;
 
 	return false;
 }
@@ -136,7 +134,7 @@ int watch_add(int fd, uint32_t events, watch_event_cb_t callback,
 	if (unlikely(fd < 0 || !callback))
 		return -EINVAL;
 
-	if (!epoll_fd)
+	if (epoll_fd < 0)
 		return -EIO;
 
 	if ((unsigned int) fd > watch_entries - 1)
@@ -284,7 +282,7 @@ int idle_add(idle_event_cb_t callback, void *user_data, uint32_t flags,
 	if (unlikely(!callback))
 		return -EINVAL;
 
-	if (!epoll_fd)
+	if (epoll_fd < 0)
 		return -EIO;
 
 	data = l_new(struct idle_data, 1);
@@ -509,7 +507,7 @@ LIB_EXPORT int l_main_run(void)
 	int timeout;
 
 	/* Has l_main_init() been called? */
-	if (unlikely(!epoll_fd))
+	if (unlikely(epoll_fd < 0))
 		return EXIT_FAILURE;
 
 	if (unlikely(epoll_running))
@@ -577,7 +575,7 @@ LIB_EXPORT bool l_main_exit(void)
 	idle_list = NULL;
 
 	close(epoll_fd);
-	epoll_fd = 0;
+	epoll_fd = -1;
 
 	return true;
 }
-- 
2.35.1

[PATCH 2/3] dhcp6-transport: Remove udp_fd field

[Greg Depoire--Ferrer]

_dhcp6_default_transport_new forgot to initialize the udp_fd field to a
negative value. Because of this, _dhcp6_default_transport_close would
try to close it. For instance, this could end up closing FD 0 (stdin) if
memory allocation returned was zeroed.

However, udp_fd is not actually used and is a left over from from the
dhcp-transport.c file that was copied to implement DHCPv6 in commit
b384838d1a79182f09b71a7a48d103ff790b8ae6 ("dhcp6: Add dhcp6 transport
abstraction and default"), so it can just be removed.
---
 ell/dhcp6-transport.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/ell/dhcp6-transport.c b/ell/dhcp6-transport.c
index 04bf4fb..8d15606 100644
--- a/ell/dhcp6-transport.c
+++ b/ell/dhcp6-transport.c
@@ -45,7 +45,6 @@
 struct dhcp6_default_transport {
 	struct dhcp6_transport super;
 	struct l_io *io;
-	int udp_fd;
 	uint16_t port;
 	struct in6_addr local;
 };
@@ -214,11 +213,6 @@ static void _dhcp6_default_transport_close(struct dhcp6_transport *s)
 
 	l_io_destroy(transport->io);
 	transport->io = NULL;
-
-	if (transport->udp_fd >= 0) {
-		L_TFR(close(transport->udp_fd));
-		transport->udp_fd = -1;
-	}
 }
 
 void _dhcp6_transport_set_rx_callback(struct dhcp6_transport *transport,
-- 
2.35.1

[PATCH 3/3] dhcp-transport: Initialize the udp_fd field

[Greg Depoire--Ferrer]

Initialize the udp_fd field to -1 in _dhcp_default_transport_new to
prevent future mistakes where a random FD is closed when the transport
is closed before the field is initialized.
---
 ell/dhcp-transport.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ell/dhcp-transport.c b/ell/dhcp-transport.c
index d73930b..8ab6327 100644
--- a/ell/dhcp-transport.c
+++ b/ell/dhcp-transport.c
@@ -550,6 +550,7 @@ struct dhcp_transport *_dhcp_default_transport_new(uint32_t ifindex,
 	transport->super.ifindex = ifindex;
 	l_strlcpy(transport->ifname, ifname, IFNAMSIZ);
 	transport->port = port;
+	transport->udp_fd = -1;
 
 	return &transport->super;
 }
-- 
2.35.1

Re: [PATCH 3/3] dhcp-transport: Initialize the udp_fd field

[Denis Kenzior]

Hi Greg,

On 5/7/22 18:54, Greg Depoire--Ferrer wrote:
> Initialize the udp_fd field to -1 in _dhcp_default_transport_new to
> prevent future mistakes where a random FD is closed when the transport
> is closed before the field is initialized.
> ---
>   ell/dhcp-transport.c | 1 +
>   1 file changed, 1 insertion(+)
> 

This mailing list isn't quite live yet, so I somehow managed to miss this 
series.  Sorry about that.  Wish I didn't since it would have saved me some time :(

Anyhow, there was a related fix applied as commit:

f5fccbab31e6 ("dhcp-transport: Do not leak fds during bind")

Regards,
-Denis

Re: Fix bug when stdin is closed

[Denis Kenzior]

Hi Greg,

On 5/7/22 18:54, Greg Depoire--Ferrer wrote:
> Running iwd with FD 0 (stdin) closed fails because ell considers epoll_fd of 0 to mean failure. The first commit fixes
> that bug.
> 
> However, it still won't work because ell/dhcp6-transport.c has a bug where it closes a uninitialized FD (FD 0 on my
> machine) and that would cause an infinite loop because the epoll_wait call was returning EBADF. The second commit fixes
> that bug.
> 
> I noticed that ell/dhcp-transport.c is prone to the same mistake as the udp_fd field is not initialized on construction,
> but I don't know if the close can actually be called before the udp_fd field is set. Just to be sure, the third commit
> initializes it.
> 

Good catch(es)!  Patches 1 & 2 applied, thanks!  See my reply regarding patch 3.

Regards,
-Denis