[PATCH 0/2] io: make sure we don't close stdio fds

[PATCH 0/2] io: make sure we don't close stdio fds

[Filip Hejsek]

The stdio chardev currently closes the stdin and stdout fds when it is
destroyed. This means that, assuming char0 is a stdio chardev, invoking
the HMP command

    chardev-change char0 null

will cause QEMU to run with descriptors 0 and 1 closed. That's not a
great situation, as:
  - the stdio chardev cannot be recreated
  - code assuming fds 0 and 1 are open will misbehave
  - the fds 0 and 1 will get assigned to newly opened files, possibly
    causing them to be confused for actual stdin/stdout

The fist patch fixes the problem by duplicating the fds before they get
passed to qio_channel_file_new_fd(), the second patch adds a warning
that is printed when qio_channel_file_new_fd() is called with a stdio
fd.

Signed-off-by: Filip Hejsek <filip.hejsek@gmail.com>
---
Filip Hejsek (2):
      char-stdio: avoid closing stdio fds
      io: report a warning when a file channel is created from a stdio fd

 chardev/char-stdio.c | 13 ++++++++++++-
 io/channel-file.c    |  6 ++++++
 2 files changed, 18 insertions(+), 1 deletion(-)
---
base-commit: 615ece3c406b262996e569f29ea6eed0d81c4d8d
change-id: 20261012-fix-stdio-close-98864055d473

Best regards,
--  
Filip Hejsek <filip.hejsek@gmail.com>

[PATCH 1/2] char-stdio: avoid closing stdio fds

[Filip Hejsek]

Use a duplicate of stdin and stdout fds to avoid closing the standard
descriptors when the stdio chardev is destroyed.

Signed-off-by: Filip Hejsek <filip.hejsek@gmail.com>
---
 chardev/char-stdio.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c
index fe1cbb2d88..79092c4a75 100644
--- a/chardev/char-stdio.c
+++ b/chardev/char-stdio.c
@@ -108,7 +108,18 @@ static bool stdio_chr_open(Chardev *chr, ChardevBackend *backend, Error **errp)
         return false;
     }
 
-    if (!qemu_chr_open_fd(chr, 0, 1, errp)) {
+    int fd_in = qemu_dup(0);
+    if (fd_in < 0) {
+        return false;
+    }
+    int fd_out = qemu_dup(1);
+    if (fd_out < 0) {
+        qemu_close(fd_in);
+        return false;
+    }
+    if (!qemu_chr_open_fd(chr, fd_in, fd_out, errp)) {
+        qemu_close(fd_in);
+        qemu_close(fd_out);
         return false;
     }
 

-- 
2.56.0

[PATCH 2/2] io: report a warning when a file channel is created from a stdio fd

[Filip Hejsek]

A channel closes its fd when the last reference to it is dropped.
Closing stdio fds is typically unwanted, so warn when they are passed to
qio_channel_file_new_fd().

Signed-off-by: Filip Hejsek <filip.hejsek@gmail.com>
---
 io/channel-file.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/io/channel-file.c b/io/channel-file.c
index 5cef75a67c..98ed8a973e 100644
--- a/io/channel-file.c
+++ b/io/channel-file.c
@@ -19,6 +19,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "io/channel-file.h"
 #include "io/channel-util.h"
 #include "io/channel-watch.h"
@@ -32,6 +33,11 @@ qio_channel_file_new_fd(int fd)
 {
     QIOChannelFile *ioc;
 
+    if (fd <= STDERR_FILENO) {
+        warn_report("potential bug: creating a channel from stdio fd %d; "
+                    "the fd will be closed when the channel is destroyed", fd);
+    }
+
     ioc = QIO_CHANNEL_FILE(object_new(TYPE_QIO_CHANNEL_FILE));
 
     ioc->fd = fd;

-- 
2.56.0

Re: [PATCH 2/2] io: report a warning when a file channel is created from a stdio fd

[marcandre.lureau]

> A channel closes its fd when the last reference to it is dropped.
> Closing stdio fds is typically unwanted, so warn when they are passed to
> qio_channel_file_new_fd().
> 
> Signed-off-by: Filip Hejsek <filip.hejsek@gmail.com>
> Message-ID: <20261013-fix-stdio-close-v1-2-4ecfa416c7bc@gmail.com>
>
> diff --git a/io/channel-file.c b/io/channel-file.c
> index 5cef75a67c62..98ed8a973e76 100644
> --- a/io/channel-file.c
> +++ b/io/channel-file.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"

place it below, with other qemu/ headers to preserve alphabetical ordering

>  #include "io/channel-file.h"
>  #include "io/channel-util.h"
>  #include "io/channel-watch.h"
> @@ -32,6 +33,11 @@ qio_channel_file_new_fd(int fd)
>  {
>      QIOChannelFile *ioc;
>  
> +    if (fd <= STDERR_FILENO) {
> +        warn_report("potential bug: creating a channel from stdio fd %d; "
> +                    "the fd will be closed when the channel is destroyed", fd);
> +    }
> +
>      ioc = QIO_CHANNEL_FILE(object_new(TYPE_QIO_CHANNEL_FILE));
>  
>      ioc->fd = fd;

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH 1/2] char-stdio: avoid closing stdio fds

[marcandre.lureau]

> Use a duplicate of stdin and stdout fds to avoid closing the standard
> descriptors when the stdio chardev is destroyed.
> 
> Signed-off-by: Filip Hejsek <filip.hejsek@gmail.com>
> Message-ID: <20261013-fix-stdio-close-v1-1-4ecfa416c7bc@gmail.com>
>
> diff --git a/chardev/char-stdio.c b/chardev/char-stdio.c
> index fe1cbb2d880f..79092c4a7562 100644
> --- a/chardev/char-stdio.c
> +++ b/chardev/char-stdio.c
> @@ -108,7 +108,18 @@ static bool stdio_chr_open(Chardev *chr, ChardevBackend *backend, Error **errp)
>          return false;
>      }
>  
> -    if (!qemu_chr_open_fd(chr, 0, 1, errp)) {
> +    int fd_in = qemu_dup(0);

QEMU style requires declarations at the beginning of blocks. Same applies to
fd_out below.

> +    if (fd_in < 0) {
> +        return false;

qemu_dup() can fail (EMFILE, EBADF). This returns false without setting *errp.
Should be:

    error_setg_errno(errp, errno, "Failed to dup stdin fd");

> +    }
> +    int fd_out = qemu_dup(1);
> +    if (fd_out < 0) {
> +        qemu_close(fd_in);
> +        return false;

Same

-- 
Marc-André Lureau <marcandre.lureau@redhat.com>