[Qemu-devel] [PATCH] qemu-sockets: do not test path with access() before unlinking

[Qemu-devel] [PATCH] qemu-sockets: do not test path with access() before unlinking

[Paolo Bonzini]

Using access() is a time-of-check/time-of-use race condition.  It is
okay to use them to provide better error messages, but that is pretty
much it.

This is not one such case, so just drop the call.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/qemu-sockets.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 9142917..2833c70 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -751,8 +751,7 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
         qemu_opt_set(opts, "path", un.sun_path, &error_abort);
     }
 
-    if ((access(un.sun_path, F_OK) == 0) &&
-        unlink(un.sun_path) < 0) {
+    if (unlink(un.sun_path) < 0) {
         error_setg_errno(errp, errno,
                          "Failed to unlink socket %s", un.sun_path);
         goto err;
-- 
2.5.0

Re: [Qemu-devel] [PATCH] qemu-sockets: do not test path with access() before unlinking

[Cole Robinson]

On 11/02/2015 09:10 AM, Paolo Bonzini wrote:
> Using access() is a time-of-check/time-of-use race condition.  It is
> okay to use them to provide better error messages, but that is pretty
> much it.
> 
> This is not one such case, so just drop the call.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  util/qemu-sockets.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 9142917..2833c70 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -751,8 +751,7 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
>          qemu_opt_set(opts, "path", un.sun_path, &error_abort);
>      }
>  
> -    if ((access(un.sun_path, F_OK) == 0) &&
> -        unlink(un.sun_path) < 0) {
> +    if (unlink(un.sun_path) < 0) {
>          error_setg_errno(errp, errno,
>                           "Failed to unlink socket %s", un.sun_path);
>          goto err;
> 

This is a serious semantic change, after this patch you will get:

$ ./x86_64-softmmu/qemu-system-x86_64 -vnc unix:/tmp/idontexist.sock
qemu-system-x86_64: -vnc unix:/tmp/idontexist.sock: Failed to start VNC
server: Failed to unlink socket /tmp/idontexist.sock: No such file or directory

Previously it would 'just work'. Common libvirt usage depends on this as well

Yeah there's a TOCTTOU race here, but it's very minor: if sun_path is created
after the access() check, qemu is just going to fail to start since bind()
will barf if the unix socket path exists.

- Cole

Re: [Qemu-devel] [PATCH] qemu-sockets: do not test path with access() before unlinking

[Markus Armbruster]

Cole Robinson <crobinso@redhat.com> writes:

> On 11/02/2015 09:10 AM, Paolo Bonzini wrote:
>> Using access() is a time-of-check/time-of-use race condition.  It is
>> okay to use them to provide better error messages, but that is pretty
>> much it.
>> 
>> This is not one such case, so just drop the call.
>> 
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  util/qemu-sockets.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> index 9142917..2833c70 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -751,8 +751,7 @@ int unix_listen_opts(QemuOpts *opts, Error **errp)
>>          qemu_opt_set(opts, "path", un.sun_path, &error_abort);
>>      }
>>  
>> -    if ((access(un.sun_path, F_OK) == 0) &&
>> -        unlink(un.sun_path) < 0) {
>> +    if (unlink(un.sun_path) < 0) {
>>          error_setg_errno(errp, errno,
>>                           "Failed to unlink socket %s", un.sun_path);
>>          goto err;
>> 
>
> This is a serious semantic change, after this patch you will get:
>
> $ ./x86_64-softmmu/qemu-system-x86_64 -vnc unix:/tmp/idontexist.sock
> qemu-system-x86_64: -vnc unix:/tmp/idontexist.sock: Failed to start VNC
> server: Failed to unlink socket /tmp/idontexist.sock: No such file or directory
>
> Previously it would 'just work'. Common libvirt usage depends on this as well
>
> Yeah there's a TOCTTOU race here, but it's very minor: if sun_path is created
> after the access() check, qemu is just going to fail to start since bind()
> will barf if the unix socket path exists.

As Paolo says, access() is almost never a solution to anything.  Here,
the proper solution should be something like

    if (unlink(un.sun_path) < 0 && errno != ENOENT) {

Re: [Qemu-devel] [PATCH] qemu-sockets: do not test path with access() before unlinking

[Paolo Bonzini]

On 02/11/2015 16:18, Cole Robinson wrote:
>> > -    if ((access(un.sun_path, F_OK) == 0) &&
>> > -        unlink(un.sun_path) < 0) {
>> > +    if (unlink(un.sun_path) < 0) {
>> >          error_setg_errno(errp, errno,
>> >                           "Failed to unlink socket %s", un.sun_path);
>> >          goto err;
>> > 
> This is a serious semantic change, after this patch you will get:
> 
> $ ./x86_64-softmmu/qemu-system-x86_64 -vnc unix:/tmp/idontexist.sock
> qemu-system-x86_64: -vnc unix:/tmp/idontexist.sock: Failed to start VNC
> server: Failed to unlink socket /tmp/idontexist.sock: No such file or directory
> 
> Previously it would 'just work'. Common libvirt usage depends on this as well
> 
> Yeah there's a TOCTTOU race here, but it's very minor: if sun_path is created
> after the access() check, qemu is just going to fail to start since bind()
> will barf if the unix socket path exists.

You're right.  I misread how to test the change.

The right change is what Markus proposed.

Paolo