diff mbox

Re: [FOR 0.12][PATCH] monitor: Accept input only byte-wise

Message ID 4BC84683.7050003@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini April 16, 2010, 11:14 a.m. UTC 
> The QEMU code appears to be written to assume that it will recvmsg() a
> complete monitor command in one go + process that, because it closes the
> FD the moment the data from any recvmsg() is dealt with.

This is buggy anyway.  This should fix it too:


Paolo

Comments

Daniel P. Berrangé April 16, 2010, 1:17 p.m. UTC | #1 
On Fri, Apr 16, 2010 at 01:14:11PM +0200, Paolo Bonzini wrote:
> 
> >The QEMU code appears to be written to assume that it will recvmsg() a
> >complete monitor command in one go + process that, because it closes the
> >FD the moment the data from any recvmsg() is dealt with.
> 
> This is buggy anyway.  This should fix it too:

Yep, this makes it work too, but if a client is evil they could
pass a FD to qemu with any other non-getfd command & it'd remain 
open for ever. Probably not important though.

Daniel
Paolo Bonzini April 16, 2010, 2:57 p.m. UTC | #2 
On 04/16/2010 03:17 PM, Daniel P. Berrange wrote:
> On Fri, Apr 16, 2010 at 01:14:11PM +0200, Paolo Bonzini wrote:
>>
>>> The QEMU code appears to be written to assume that it will recvmsg() a
>>> complete monitor command in one go + process that, because it closes the
>>> FD the moment the data from any recvmsg() is dealt with.
>>
>> This is buggy anyway.  This should fix it too:
>
> Yep, this makes it work too, but if a client is evil they could
> pass a FD to qemu with any other non-getfd command&  it'd remain
> open for ever. Probably not important though.

No, it wouldn't: outside the part that I patched there is this:

         if (s->msgfd != -1)
             close(s->msgfd);
         s->msgfd = fd;

Only one file descriptor could "leak".

Paolo
diff mbox

Patch

diff --git a/monitor.c b/monitor.c
index 5659991..225a922 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2408,15 +2408,6 @@ 
          return -1;
      }

-    fd = dup(fd);
-    if (fd == -1) {
-        if (errno == EMFILE)
-            qerror_report(QERR_TOO_MANY_FILES);
-        else
-            qerror_report(QERR_UNDEFINED_ERROR);
-        return -1;
-    }
-
      QLIST_FOREACH(monfd, &mon->fds, next) {
          if (strcmp(monfd->name, fdname) != 0) {
              continue;
diff --git a/qemu-char.c b/qemu-char.c
index 05df971..ac65a1c 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2000,8 +2000,9 @@ 
  static int tcp_get_msgfd(CharDriverState *chr)
  {
      TCPCharDriver *s = chr->opaque;
-
-    return s->msgfd;
+    int fd = s->msgfd;
+    s->msgfd = -1;
+    return fd;
  }

  #ifndef _WIN32
@@ -2089,10 +2090,6 @@  static void tcp_chr_read(void *opaque)
              tcp_chr_process_IAC_bytes(chr, s, buf, &size);
          if (size > 0)
              qemu_chr_read(chr, buf, size);
-        if (s->msgfd != -1) {
-            close(s->msgfd);
-            s->msgfd = -1;
-        }
      }
  }