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

[Markus Armbruster]

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.