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

[Eric Blake]

On 05/24/2017 10:54 AM, Eric Blake wrote:
> On 05/24/2017 10:42 AM, Daniel P. Berrange wrote:
>> The 'struct sockaddr_un' only allows 108 bytes for the socket
>> path. Currently QEMU uses snprintf() and so silently truncates
>> the socket path provided by the user. This is undesirable because
>> the user will then be unable to connect to the path they asked
>> for. This change makes QEMU bounds check and report an explicit
>> error message.
>>
>> Signed-off-by: Daniel P. Berrange <address@hidden>
>> ---
>>  util/qemu-sockets.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>

>>          snprintf(un.sun_path, sizeof(un.sun_path), "%s", saddr->path);
> 
> Pre-existing, but now that we know things fit, isn't it faster to use
> strcpy (or strpcpy or strncpy) instead of the overhead of snprintf?
> 
> You're on the right track, but it may be worth a v2 for further cleanups.

Oh, and while there, I noticed:

        tmpdir = tmpdir ? tmpdir : "/tmp";
        if (snprintf(un.sun_path, sizeof(un.sun_path),
"%s/qemu-socket-XXXXXX",
                     tmpdir) >= sizeof(un.sun_path)) {
            error_setg_errno(errp, errno,
                             "TMPDIR environment variable (%s) too
large", tmpdir);

but 'errno' is NOT required to be set when snprintf() returns a larger
value, so our error message is (likely) garbage.  Better might be
calling error_setg_errno(errp, ENAMETOOLONG, ...), or just living with
the simpler error_setg().

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

signature.asc
Description: OpenPGP digital signature