* Fix bug when stdin is closed
@ 2022-05-07 23:54 Greg Depoire--Ferrer
2022-05-07 23:54 ` [PATCH 1/3] main: Accept FD 0 as epoll_fd Greg Depoire--Ferrer
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Greg Depoire--Ferrer @ 2022-05-07 23:54 UTC (permalink / raw)
To: ell
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] main: Accept FD 0 as epoll_fd
2022-05-07 23:54 Fix bug when stdin is closed Greg Depoire--Ferrer
@ 2022-05-07 23:54 ` Greg Depoire--Ferrer
2022-05-07 23:54 ` [PATCH 2/3] dhcp6-transport: Remove udp_fd field Greg Depoire--Ferrer
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Greg Depoire--Ferrer @ 2022-05-07 23:54 UTC (permalink / raw)
To: ell; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] dhcp6-transport: Remove udp_fd field
2022-05-07 23:54 Fix bug when stdin is closed Greg Depoire--Ferrer
2022-05-07 23:54 ` [PATCH 1/3] main: Accept FD 0 as epoll_fd Greg Depoire--Ferrer
@ 2022-05-07 23:54 ` Greg Depoire--Ferrer
2022-05-07 23:54 ` [PATCH 3/3] dhcp-transport: Initialize the " Greg Depoire--Ferrer
2022-06-06 18:18 ` Fix bug when stdin is closed Denis Kenzior
3 siblings, 0 replies; 6+ messages in thread
From: Greg Depoire--Ferrer @ 2022-05-07 23:54 UTC (permalink / raw)
To: ell; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] dhcp-transport: Initialize the udp_fd field
2022-05-07 23:54 Fix bug when stdin is closed Greg Depoire--Ferrer
2022-05-07 23:54 ` [PATCH 1/3] main: Accept FD 0 as epoll_fd Greg Depoire--Ferrer
2022-05-07 23:54 ` [PATCH 2/3] dhcp6-transport: Remove udp_fd field Greg Depoire--Ferrer
@ 2022-05-07 23:54 ` Greg Depoire--Ferrer
2022-06-06 18:17 ` Denis Kenzior
2022-06-06 18:18 ` Fix bug when stdin is closed Denis Kenzior
3 siblings, 1 reply; 6+ messages in thread
From: Greg Depoire--Ferrer @ 2022-05-07 23:54 UTC (permalink / raw)
To: ell; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] dhcp-transport: Initialize the udp_fd field
2022-05-07 23:54 ` [PATCH 3/3] dhcp-transport: Initialize the " Greg Depoire--Ferrer
@ 2022-06-06 18:17 ` Denis Kenzior
0 siblings, 0 replies; 6+ messages in thread
From: Denis Kenzior @ 2022-06-06 18:17 UTC (permalink / raw)
To: Greg Depoire--Ferrer, ell
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Fix bug when stdin is closed
2022-05-07 23:54 Fix bug when stdin is closed Greg Depoire--Ferrer
` (2 preceding siblings ...)
2022-05-07 23:54 ` [PATCH 3/3] dhcp-transport: Initialize the " Greg Depoire--Ferrer
@ 2022-06-06 18:18 ` Denis Kenzior
3 siblings, 0 replies; 6+ messages in thread
From: Denis Kenzior @ 2022-06-06 18:18 UTC (permalink / raw)
To: Greg Depoire--Ferrer, ell
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-06 18:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-07 23:54 Fix bug when stdin is closed Greg Depoire--Ferrer
2022-05-07 23:54 ` [PATCH 1/3] main: Accept FD 0 as epoll_fd Greg Depoire--Ferrer
2022-05-07 23:54 ` [PATCH 2/3] dhcp6-transport: Remove udp_fd field Greg Depoire--Ferrer
2022-05-07 23:54 ` [PATCH 3/3] dhcp-transport: Initialize the " Greg Depoire--Ferrer
2022-06-06 18:17 ` Denis Kenzior
2022-06-06 18:18 ` Fix bug when stdin is closed Denis Kenzior
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).