Hi Aleksander,
On 20/06/18 22:56, Aleksander Morgado wrote: > Hey, > > On Wed, Jun 20, 2018 at 12:25 AM, Brian, Sam <[email protected]> wrote: >> After the tty is opened, the fd is passed off to a g_io_channel which >> manages it. When the serial port is closed with _close_internal(), >> g_io_channel_shutdown() is called which then calls g_io_unix_close() which >> in turn close()s the fd. >> Currently, _close_internal() performs a second explicit close() after the >> g_io_channel shutdown which *in the best case scenario* will fail with EBADF. >> This is a race condition that can result in other file descriptors being >> closed. >> >> This change avoids double closing the serial port fd. >> --- >> >> Hey, >> I had this problem with a Telit LE910-EU1. What do you think of the fix? >> >> To give some more detail about the problem... >> >> The g_io_channel_shutdown() calling close() can be verified with >> this short program: >> >> $ cat test.c >> #include <glib.h> >> #include <errno.h> >> #include <stdio.h> >> #include <unistd.h> >> >> int main (int argc, char* argv[]) >> { >> int ret; >> GIOChannel *channel; >> >> channel = g_io_channel_unix_new(STDIN_FILENO); >> g_io_channel_shutdown(channel, FALSE, NULL); >> >> ret = close(STDIN_FILENO); >> printf("close() -> %d: %m\n", ret); >> return 0; >> } >> >> $ gcc -o test test.c -g $(pkg-config --cflags --libs gio-2.0) >> $ ./test >> close() -> -1: Bad file descriptor >> >> Here is a snippet of strace from ModemManager that demonstrates when the >> race causes another fd to be closed: >> >> ### Close the tty (from g_io_channel_shutdown) >> [pid 413] close(9 <unfinished ...> >> [pid 687] <... poll resumed> ) = 1 ([{fd=7, revents=POLLIN}]) >> ... >> ### tty's closed fd is reused by eventfd2 >> [pid 687] SYS_356(0, 0x80800, 0xc6c4c8, 0, 0xc6c4d0) = 9 >> ... >> [pid 687] poll([{fd=7, events=POLLIN}, {fd=6, events=POLLIN}, {fd=9, >> events=POLLIN}], 3, -1 <unfinished ...> >> ### First close succeeds (tty fd) >> [pid 413] <... close resumed> ) = 0 >> ### Second (explicit) close succeeds (eventfd fd, oops) >> [pid 413] close(9) = 0 >> >> PID 413 is ModemManager, PID 687 is a clone process created with the flag >> CLONE_FILES so the fd table is shared between parent and clone. >> From the clone's read/writes, it appears to be used to communicate with >> dbus. >> >> Strangely, closing the eventfd didn't always cause a noticable failure. >> It was only when *somehow* the port-serial code tried to use an eventfd as >> a tty and AT commands fail. >> >> src/mm-port-serial.c | 24 +++++++++++++----------- >> 1 file changed, 13 insertions(+), 11 deletions(-) >> >> diff --git a/src/mm-port-serial.c b/src/mm-port-serial.c >> index 44699a76..9ba4e5d2 100644 >> --- a/src/mm-port-serial.c >> +++ b/src/mm-port-serial.c >> @@ -1262,6 +1262,12 @@ success: >> error: >> mm_warn ("(%s) failed to open serial device", device); >> >> + if (self->priv->fd >= 0) { >> + if (!self->priv->iochannel) >> + close (self->priv->fd); >> + self->priv->fd = -1; >> + } >> + >> if (self->priv->iochannel) { >> g_io_channel_shutdown (self->priv->iochannel, FALSE, NULL); >> g_io_channel_unref (self->priv->iochannel); >> @@ -1274,11 +1280,6 @@ error: >> self->priv->socket = NULL; >> } >> >> - if (self->priv->fd >= 0) { >> - close (self->priv->fd); >> - self->priv->fd = -1; >> - } >> - >> return FALSE; >> } >> >> @@ -1348,6 +1349,13 @@ _close_internal (MMPortSerial *self, gboolean force) >> tcflush (self->priv->fd, TCIOFLUSH); >> } >> >> + /* Close fd if not part of the iochannel (shutdown closes fd) */ >> + if (self->priv->fd >= 0) { >> + if (!self->priv->iochannel) >> + close (self->priv->fd); >> + self->priv->fd = -1; >> + } >> + >> /* Destroy channel */ >> if (self->priv->iochannel) { >> data_watch_enable (self, FALSE); >> @@ -1356,12 +1364,6 @@ _close_internal (MMPortSerial *self, gboolean force) >> self->priv->iochannel = NULL; >> } >> >> - /* Close fd, if any */ >> - if (self->priv->fd >= 0) { >> - close (self->priv->fd); >> - self->priv->fd = -1; >> - } >> - >> /* Destroy socket */ >> if (self->priv->socket) { >> data_watch_enable (self, FALSE); > > I believe the analysis is good indeed. When we create a GIOChannel > from a FD, there is nothing in the documentation saying that > g_io_channel_shutdown() won't close the underlying FD. There is an > explicit mention to g_io_channel_unref() not implicitly closing it, > but nothing about shutdown. Following the GLib code, I see how > g_io_channel_shutdown() will call the "io_close" method in the > GIOFuncs table, which for the unix channel will run g_io_unix_close(), > always doing an explicit close(). I didn't find the g_io_channel_shutdown() documentation immediately obvious either. The strace tipped me off, and then I followed the code. > > Why not change the logic and instead of not closing when there is no > iochannel, we just remove the g_io_channel_shutdown() calls? We know > that g_io_channel_unref() won't do any implicit close, so we can > safely destroy the iochannel regardless of whether we leave the > underlying FD open, and then close the FD with close() ourselves > unconditionally. That would probably be easier to understand as well? > Instead of the shutdown() call we could place a big fat /* comment */ > explaining why we don't call shutdown. > > What do you think? > If we directly close() instead of using g_io_channel_shutdown() then (1) the channel buffer won't be flushed before closing (which is probably fine?), and (2) if there are other refs to the channel and the "closing" g_io_channel_unref() doesn't destroy it, then the channel's internal record keeping will think that the closed fd is still open and readable/writable. I know (2) is not a problem today because there are no other g_io_channel_ref() calls, but it could be overlooked if one was added in future. Perhaps a better way would be to only use g_io_channel_shutdown() and replace priv->fd with g_io_channel_unix_get_fd() where required for the tcgetattr/tcsetattr/ioctl? Removing priv->fd would mean the fd is tracked in only one place. The "self->priv->fd >= 0" checks would become the "self->priv->iochannel" check so the fd being open would be tied directly to the iochannel being present. Sam _______________________________________________ ModemManager-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
