Patchwork [v5,1/6] qemu-char: Add MSG_CMSG_CLOEXEC flag to recvmsg

login
register
mail settings
Submitter Corey Bryant
Date July 23, 2012, 1:08 p.m.
Message ID <1343048885-1701-2-git-send-email-coreyb@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/172678/
State New
Headers show

Comments

Corey Bryant - July 23, 2012, 1:08 p.m.
Set the close-on-exec flag for the file descriptor received
via SCM_RIGHTS.

v4
 -This patch is new in v4 (eblake@redhat.com)

v5
 -Fallback to FD_CLOEXEC if MSG_CMSG_CLOEXEC is not available
  (eblake@redhat.com, stefanha@linux.vnet.ibm.com)

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
 qemu-char.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
Eric Blake - July 23, 2012, 10:50 p.m.
On 07/23/2012 07:08 AM, Corey Bryant wrote:
> Set the close-on-exec flag for the file descriptor received
> via SCM_RIGHTS.
> 

> +++ b/qemu-char.c
> @@ -2263,9 +2263,17 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
>      msg.msg_control = &msg_control;
>      msg.msg_controllen = sizeof(msg_control);
>  
> +#ifdef MSG_CMSG_CLOEXEC
> +    ret = recvmsg(s->fd, &msg, MSG_CMSG_CLOEXEC);
> +#else
>      ret = recvmsg(s->fd, &msg, 0);
> -    if (ret > 0 && s->is_unix)
> +    if (ret > 0) {
> +        qemu_set_cloexec(s->fd);

Wrong fd.  You aren't changing cloexec on the socket (s->fd), but on the
fd that was received via msg (which you don't know at this point in time).

> +    }
> +#endif
> +    if (ret > 0 && s->is_unix) {
>          unix_process_msgfd(chr, &msg);

Only here do you know what fd you received.

I would write it more like:

int flags = 0;
#ifdef MSG_CMSG_CLOEXEC
flags |= MSG_CMSG_CLOEXEC
#endif
ret = recvmsg(s->fd, &msg, flags);
if (ret > 0 && s->is_unix) {
    unix_process_msgfd(chr, &msg);
#ifndef MSG_CMSG_CLOEXEC
    qemu_set_cloexec(/* fd determined from msg */)
#endif
}

which almost implies that unix_process_msgfd() should be the function
that sets cloexec, but without wasting the time doing so if recvmsg
already did the job.
Corey Bryant - July 24, 2012, 2:19 a.m.
On 07/23/2012 06:50 PM, Eric Blake wrote:
> On 07/23/2012 07:08 AM, Corey Bryant wrote:
>> Set the close-on-exec flag for the file descriptor received
>> via SCM_RIGHTS.
>>
>
>> +++ b/qemu-char.c
>> @@ -2263,9 +2263,17 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
>>       msg.msg_control = &msg_control;
>>       msg.msg_controllen = sizeof(msg_control);
>>
>> +#ifdef MSG_CMSG_CLOEXEC
>> +    ret = recvmsg(s->fd, &msg, MSG_CMSG_CLOEXEC);
>> +#else
>>       ret = recvmsg(s->fd, &msg, 0);
>> -    if (ret > 0 && s->is_unix)
>> +    if (ret > 0) {
>> +        qemu_set_cloexec(s->fd);
>
> Wrong fd.  You aren't changing cloexec on the socket (s->fd), but on the
> fd that was received via msg (which you don't know at this point in time).
>

Ugh, that's bad.

>> +    }
>> +#endif
>> +    if (ret > 0 && s->is_unix) {
>>           unix_process_msgfd(chr, &msg);
>
> Only here do you know what fd you received.
>
> I would write it more like:
>
> int flags = 0;
> #ifdef MSG_CMSG_CLOEXEC
> flags |= MSG_CMSG_CLOEXEC
> #endif
> ret = recvmsg(s->fd, &msg, flags);
> if (ret > 0 && s->is_unix) {
>      unix_process_msgfd(chr, &msg);
> #ifndef MSG_CMSG_CLOEXEC
>      qemu_set_cloexec(/* fd determined from msg */)
> #endif
> }
>
> which almost implies that unix_process_msgfd() should be the function
> that sets cloexec, but without wasting the time doing so if recvmsg
> already did the job.
>

Thanks for the suggestion and catching this.  I'll take this into 
account in the next version.

Patch

diff --git a/qemu-char.c b/qemu-char.c
index c2aaaee..eedf66d 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2263,9 +2263,17 @@  static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
     msg.msg_control = &msg_control;
     msg.msg_controllen = sizeof(msg_control);
 
+#ifdef MSG_CMSG_CLOEXEC
+    ret = recvmsg(s->fd, &msg, MSG_CMSG_CLOEXEC);
+#else
     ret = recvmsg(s->fd, &msg, 0);
-    if (ret > 0 && s->is_unix)
+    if (ret > 0) {
+        qemu_set_cloexec(s->fd);
+    }
+#endif
+    if (ret > 0 && s->is_unix) {
         unix_process_msgfd(chr, &msg);
+    }
 
     return ret;
 }