[PATCH v4 05/12] contrib/vhost-user-blk: fix bind() using the right size of the address

Stefano Garzarella posted 12 patches 6 months, 3 weeks ago
Maintainers: David Hildenbrand <david@redhat.com>, Igor Mammedov <imammedo@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Raphael Norwitz <raphael@enfabrica.net>, Gerd Hoffmann <kraxel@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, Jason Wang <jasowang@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Thomas Huth <thuth@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Coiby Xu <Coiby.Xu@gmail.com>, Laurent Vivier <lvivier@redhat.com>
There is a newer version of this series
[PATCH v4 05/12] contrib/vhost-user-blk: fix bind() using the right size of the address
Posted by Stefano Garzarella 6 months, 3 weeks ago
On macOS passing `-s /tmp/vhost.socket` parameter to the vhost-user-blk
application, the bind was done on `/tmp/vhost.socke` pathname,
missing the last character.

This sounds like one of the portability problems described in the
unix(7) manpage:

   Pathname sockets
       When  binding  a socket to a pathname, a few rules should
       be observed for maximum portability and ease of coding:

       •  The pathname in sun_path should be null-terminated.

       •  The length of the pathname, including the  terminating
          null byte, should not exceed the size of sun_path.

       •  The  addrlen  argument  that  describes  the enclosing
          sockaddr_un structure should have a value of at least:

              offsetof(struct sockaddr_un, sun_path) +
              strlen(addr.sun_path)+1

          or,  more  simply,  addrlen  can   be   specified   as
          sizeof(struct sockaddr_un).

So let's follow the last advice and simplify the code as well.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 contrib/vhost-user-blk/vhost-user-blk.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
index 89e5f11a64..a8ab9269a2 100644
--- a/contrib/vhost-user-blk/vhost-user-blk.c
+++ b/contrib/vhost-user-blk/vhost-user-blk.c
@@ -469,7 +469,6 @@ static int unix_sock_new(char *unix_fn)
 {
     int sock;
     struct sockaddr_un un;
-    size_t len;
 
     assert(unix_fn);
 
@@ -481,10 +480,9 @@ static int unix_sock_new(char *unix_fn)
 
     un.sun_family = AF_UNIX;
     (void)snprintf(un.sun_path, sizeof(un.sun_path), "%s", unix_fn);
-    len = sizeof(un.sun_family) + strlen(un.sun_path);
 
     (void)unlink(unix_fn);
-    if (bind(sock, (struct sockaddr *)&un, len) < 0) {
+    if (bind(sock, (struct sockaddr *)&un, sizeof(un)) < 0) {
         perror("bind");
         goto fail;
     }
-- 
2.45.0


Re: [PATCH v4 05/12] contrib/vhost-user-blk: fix bind() using the right size of the address
Posted by Philippe Mathieu-Daudé 6 months, 3 weeks ago
On 8/5/24 09:44, Stefano Garzarella wrote:
> On macOS passing `-s /tmp/vhost.socket` parameter to the vhost-user-blk
> application, the bind was done on `/tmp/vhost.socke` pathname,
> missing the last character.
> 
> This sounds like one of the portability problems described in the
> unix(7) manpage:
> 
>     Pathname sockets
>         When  binding  a socket to a pathname, a few rules should
>         be observed for maximum portability and ease of coding:
> 
>         •  The pathname in sun_path should be null-terminated.
> 
>         •  The length of the pathname, including the  terminating
>            null byte, should not exceed the size of sun_path.
> 
>         •  The  addrlen  argument  that  describes  the enclosing
>            sockaddr_un structure should have a value of at least:
> 
>                offsetof(struct sockaddr_un, sun_path) +
>                strlen(addr.sun_path)+1
> 
>            or,  more  simply,  addrlen  can   be   specified   as
>            sizeof(struct sockaddr_un).
> 
> So let's follow the last advice and simplify the code as well.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>   contrib/vhost-user-blk/vhost-user-blk.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)

Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>