Re: [Qemu-devel] [PATCH v3] sockets: improve error reporting if UNIX socket path is too long

[Eric Blake]

On 05/25/2017 09:40 AM, Daniel P. Berrange wrote:
> On Thu, May 25, 2017 at 09:28:31AM -0500, Eric Blake wrote:
>> On 05/25/2017 05:19 AM, Daniel P. Berrange wrote:
>>> The 'struct sockaddr_un' only allows 108 bytes for the socket
>>> path.
>>>
>>> If the user supplies a path, QEMU uses snprintf() to silently
>>> truncate it when too long. This is undesirable because the user
>>> will then be unable to connect to the path they asked for.
>>>
>>> If the user doesn't supply a path, QEMU builds one based on
>>> TMPDIR, but if that leads to an overlong path, it mistakenly
>>> uses error_setg_errno() with a stale errno value, because
>>> snprintf() does not set errno on truncation.
>>>
>>> In solving this the code needed some refactoring to ensure we
>>> don't pass 'un.sun_path' directly to any APIs which expect
>>> NUL-terminated strings, because the path is not required to
>>> be terminated.
>>>
>>> Signed-off-by: Daniel P. Berrange <address@hidden>

>>>  
>>> -    if (unlink(un.sun_path) < 0 && errno != ENOENT) {
>>> +    if (strlen(path) > sizeof(un.sun_path)) {
>>> +        error_setg(errp, "UNIX socket path '%s' is too long", path);
>>> +        error_append_hint(errp, "Path must be less than %zu bytes\n",
>>> +                          sizeof(un.sun_path));
>>
>> The old message mentioned the name that was too long, the new does not.
>> That's a potential slight loss in error message quality, particularly
>> since you are no longer mentioning a UNIX socket (which may give the
>> user a clue why 108 is special), but I can probably live with it (it's
>> still pretty obvious from the message that you should investigate what
>> is too long).
> 
> I'm not sure why you are saying I don't mention the path or that it
> is a UNIX socket - I do exactly that right there.

Oh shoot, I was misreading things.  I saw two lines of error_*, and must
have thought that the first was the deletion and the second an
insertion. But you're right that they are both insertions, that the
first is an accurate message, and the second is a hint.

Never mind, I'm obviously not fully awake yet today ;)


>>> @@ -932,9 +942,16 @@ static int unix_connect_saddr(UnixSocketAddress *saddr,
>>>          qemu_set_nonblock(sock);
>>>      }
>>>  
>>> +    if (strlen(saddr->path) > sizeof(un.sun_path)) {
>>> +        error_setg(errp, "UNIX socket path '%s' is too long", saddr->path);
>>> +        error_append_hint(errp, "Path must be less than %zu bytes\n",
>>> +                          sizeof(un.sun_path));
>>
>> Again, potential loss in error message quality.
> 
> I don't think so.

Yep, another instance of me reading the patch wrong.  This part's fine,
after all.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature