diff mbox series

[08/11] sockets: Fix socket_sockaddr_to_address_unix() for abstract sockets

Message ID 20201029133833.3450220-9-armbru@redhat.com
State New, archived
Headers show
Series sockets: Attempt to drain the abstract socket swamp | expand

Commit Message

Markus Armbruster Oct. 29, 2020, 1:38 p.m. UTC
Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
support" neglected to update socket_sockaddr_to_address_unix().  The
function returns a non-abstract socket address for abstract
sockets (wrong) with a null @path (also wrong; a non-optional QAPI str
member must never be null).

The null @path is due to confused code going back all the way to
commit 17c55decec "sockets: add helpers for creating SocketAddress
from a socket".

Add the required special case, and simplify the confused code.

Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-sockets.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Paolo Bonzini Oct. 29, 2020, 5:47 p.m. UTC | #1
On 29/10/20 14:38, Markus Armbruster wrote:
> +        /* Linux abstract socket */
> +        addr->u.q_unix.path = g_strndup(su->sun_path + 1,
> +                                        sizeof(su->sun_path) - 1);
> +        addr->u.q_unix.has_abstract = true;
> +        addr->u.q_unix.abstract = true;
> +        addr->u.q_unix.has_tight = true;
> +        addr->u.q_unix.tight = !su->sun_path[sizeof(su->sun_path) - 1];
> +        return addr;

I think this should be

    addr->u.q_unit.tight = salen < sizeof(*su);

Paolo
Eric Blake Oct. 29, 2020, 7:38 p.m. UTC | #2
On 10/29/20 8:38 AM, Markus Armbruster wrote:
> Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
> support" neglected to update socket_sockaddr_to_address_unix().  The
> function returns a non-abstract socket address for abstract
> sockets (wrong) with a null @path (also wrong; a non-optional QAPI str
> member must never be null).
> 
> The null @path is due to confused code going back all the way to
> commit 17c55decec "sockets: add helpers for creating SocketAddress
> from a socket".
> 
> Add the required special case, and simplify the confused code.
> 
> Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  util/qemu-sockets.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index c802d5aa0a..801c5e3957 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1264,10 +1264,20 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
>  
>      addr = g_new0(SocketAddress, 1);
>      addr->type = SOCKET_ADDRESS_TYPE_UNIX;
> -    if (su->sun_path[0]) {
> -        addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path));
> +#ifdef CONFIG_LINUX
> +    if (!su->sun_path[0]) {
> +        /* Linux abstract socket */
> +        addr->u.q_unix.path = g_strndup(su->sun_path + 1,
> +                                        sizeof(su->sun_path) - 1);
> +        addr->u.q_unix.has_abstract = true;
> +        addr->u.q_unix.abstract = true;
> +        addr->u.q_unix.has_tight = true;
> +        addr->u.q_unix.tight = !su->sun_path[sizeof(su->sun_path) - 1];

This is questionable - how can you tell from the last byte whether the
name was created as tight or not?

> +        return addr;
>      }
> +#endif
>  
> +    addr->u.q_unix.path = g_strdup(su->sun_path);

This is wrong on at least Linux, where su->sun_path need not be
NUL-terminated (allowing file-system Unix sockets to have one more byte
in their name); you need the strndup that you replaced above, in order
avoid reading beyond the end of the array.
Markus Armbruster Oct. 30, 2020, 8:56 a.m. UTC | #3
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 29/10/20 14:38, Markus Armbruster wrote:
>> +        /* Linux abstract socket */
>> +        addr->u.q_unix.path = g_strndup(su->sun_path + 1,
>> +                                        sizeof(su->sun_path) - 1);
>> +        addr->u.q_unix.has_abstract = true;
>> +        addr->u.q_unix.abstract = true;
>> +        addr->u.q_unix.has_tight = true;
>> +        addr->u.q_unix.tight = !su->sun_path[sizeof(su->sun_path) - 1];
>> +        return addr;
>
> I think this should be
>
>     addr->u.q_unit.tight = salen < sizeof(*su);
>
> Paolo

You're right, my code is wrong.

The case "@path just fits" is ambiguous: @tight doesn't matter then.
Your code arbitrarily picks tight=false then.  Picking the default
tight=true would perhaps be a bit nicer.  Not worth complicating the
code.
Markus Armbruster Oct. 30, 2020, 9:04 a.m. UTC | #4
Eric Blake <eblake@redhat.com> writes:

> On 10/29/20 8:38 AM, Markus Armbruster wrote:
>> Commit 776b97d360 "qemu-sockets: add abstract UNIX domain socket
>> support" neglected to update socket_sockaddr_to_address_unix().  The
>> function returns a non-abstract socket address for abstract
>> sockets (wrong) with a null @path (also wrong; a non-optional QAPI str
>> member must never be null).
>> 
>> The null @path is due to confused code going back all the way to
>> commit 17c55decec "sockets: add helpers for creating SocketAddress
>> from a socket".
>> 
>> Add the required special case, and simplify the confused code.
>> 
>> Fixes: 776b97d3605ed0fc94443048fdf988c7725e38a9
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  util/qemu-sockets.c | 14 ++++++++++++--
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>> 
>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> index c802d5aa0a..801c5e3957 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -1264,10 +1264,20 @@ socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
>>  
>>      addr = g_new0(SocketAddress, 1);
>>      addr->type = SOCKET_ADDRESS_TYPE_UNIX;
>> -    if (su->sun_path[0]) {
>> -        addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path));
>> +#ifdef CONFIG_LINUX
>> +    if (!su->sun_path[0]) {
>> +        /* Linux abstract socket */
>> +        addr->u.q_unix.path = g_strndup(su->sun_path + 1,
>> +                                        sizeof(su->sun_path) - 1);
>> +        addr->u.q_unix.has_abstract = true;
>> +        addr->u.q_unix.abstract = true;
>> +        addr->u.q_unix.has_tight = true;
>> +        addr->u.q_unix.tight = !su->sun_path[sizeof(su->sun_path) - 1];
>
> This is questionable - how can you tell from the last byte whether the
> name was created as tight or not?

I plead temporary insanity.  See my reply to Paolo.

>> +        return addr;
>>      }
>> +#endif
>>  
>> +    addr->u.q_unix.path = g_strdup(su->sun_path);
>
> This is wrong on at least Linux, where su->sun_path need not be
> NUL-terminated (allowing file-system Unix sockets to have one more byte
> in their name);

Out of curiosity: is this usage portable?  I tried man pages and SUS, no
luck.

>                 you need the strndup that you replaced above, in order
> avoid reading beyond the end of the array.

You're right.  Prone to allocate a bit more than necessary (always
sizeof(su->sun_path) + 1 bytes), but that doesn't matter.
Eric Blake Oct. 30, 2020, 12:39 p.m. UTC | #5
On 10/30/20 4:04 AM, Markus Armbruster wrote:

>>> +    addr->u.q_unix.path = g_strdup(su->sun_path);
>>
>> This is wrong on at least Linux, where su->sun_path need not be
>> NUL-terminated (allowing file-system Unix sockets to have one more byte
>> in their name);
> 
> Out of curiosity: is this usage portable?  I tried man pages and SUS, no
> luck.

On Linux, 'man 7 unix' says:

> BUGS
>        When binding a socket to an address, Linux is one  of  the  implementa‐
>        tions  that  appends a null terminator if none is supplied in sun_path.
>        In most cases  this  is  unproblematic:  when  the  socket  address  is
>        retrieved,  it  will  be  one  byte  longer than that supplied when the
>        socket was bound.  However, there is one case where confusing  behavior
>        can  result: if 108 non-null bytes are supplied when a socket is bound,
>        then the addition of the null terminator takes the length of the  path‐
>        name beyond sizeof(sun_path).  Consequently, when retrieving the socket
>        address (for example, via accept(2)), if the input addrlen argument for
>        the  retrieving  call  is specified as sizeof(struct sockaddr_un), then
>        the  returned  address  structure  won't  have  a  null  terminator  in
>        sun_path.
> 
>        In  addition, some implementations don't require a null terminator when
>        binding a socket (the addrlen argument is used to determine the  length
>        of  sun_path)  and when the socket address is retrieved on these imple‐
>        mentations, there is no null terminator in sun_path.

along with advice on using strnlen and/or overallocation to handle
various cases in a cleaner manner, and the caveat that if you always use
a name smaller than sun_path you can avoid the tricky code (at the
expense of one byte less in your namespace).
diff mbox series

Patch

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index c802d5aa0a..801c5e3957 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1264,10 +1264,20 @@  socket_sockaddr_to_address_unix(struct sockaddr_storage *sa,
 
     addr = g_new0(SocketAddress, 1);
     addr->type = SOCKET_ADDRESS_TYPE_UNIX;
-    if (su->sun_path[0]) {
-        addr->u.q_unix.path = g_strndup(su->sun_path, sizeof(su->sun_path));
+#ifdef CONFIG_LINUX
+    if (!su->sun_path[0]) {
+        /* Linux abstract socket */
+        addr->u.q_unix.path = g_strndup(su->sun_path + 1,
+                                        sizeof(su->sun_path) - 1);
+        addr->u.q_unix.has_abstract = true;
+        addr->u.q_unix.abstract = true;
+        addr->u.q_unix.has_tight = true;
+        addr->u.q_unix.tight = !su->sun_path[sizeof(su->sun_path) - 1];
+        return addr;
     }
+#endif
 
+    addr->u.q_unix.path = g_strdup(su->sun_path);
     return addr;
 }
 #endif /* WIN32 */