Re: [Qemu-devel] [PATCH 08/14] nbd/client: Refactor nbd_receive_list()

[Vladimir Sementsov-Ogievskiy]

06.12.2018 19:31, Eric Blake wrote:
> On 12/6/18 8:18 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 01.12.2018 1:03, Eric Blake wrote:
>>> Add some parameters to make this function reusable in upcoming
>>> export listing, where we will want to capture the name and
>>> description rather than compare against a user-supplied name.
>>> No change in semantics to the existing caller.
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>> ---
>>>    nbd/client.c | 66 +++++++++++++++++++++++++++++++++++++---------------
>>>    1 file changed, 47 insertions(+), 19 deletions(-)
> 
>>> @@ -290,30 +298,49 @@ static int nbd_receive_list(QIOChannel *ioc, const 
>>> char *want, bool *match,
>>>            nbd_send_opt_abort(ioc);
>>>            return -1;
>>>        }
>>> -    if (namelen != strlen(want)) {
>>> -        if (nbd_drop(ioc, len, errp) < 0) {
>>> -            error_prepend(errp,
>>> -                          "failed to skip export name with wrong length: 
>>> ");
>>> -            nbd_send_opt_abort(ioc);
>>> -            return -1;
>>> +    if (want) {
>>> +        if (namelen != strlen(want)) {
>>> +            if (nbd_drop(ioc, len, errp) < 0) {
>>> +                error_prepend(errp,
>>> +                              "failed to skip export name with wrong 
>>> length: ");
>>> +                nbd_send_opt_abort(ioc);
>>> +                return -1;
>>> +            }
>>> +            return 1;
>>>            }
>>> -        return 1;
>>> +        assert(namelen < sizeof(array));
>>> +    } else {
>>> +        assert(nameout);
>>
>> this assert looks a bit redundant, if nameout is 0, next line will abort not 
>> less clearly
>>
>>> +        *nameout = name = g_new(char, namelen + 1);
>>
>> We should check namelen <= NBD_MAX_NAME_SIZE before it, I think.
> 
> Why? We already validated

Ah, than OK, the worst case is ~ NBD_MAX_BUFFER_SIZE (32M), not 4G. Missed that 
:(

  that the overall option is not too large, and even if the resulting name from 
the server is larger than NBD_MAX_NAME_SIZE, it's still nice to report that 
name to the client for 'qemu-nbd --list'.  And these days, we only call 
nbd_receive_list if a server lacked NBD_OPT_GO, which is getting rarer and 
rarer, so micro-optimizing a rare case to avoid a large malloc isn't going to 
make a noticeable difference.
> 
>>
>>>        }
>>>
>>> -    assert(namelen < sizeof(name));
>>>        if (nbd_read(ioc, name, namelen, errp) < 0) {
>>>            error_prepend(errp, "failed to read export name: ");
>>>            nbd_send_opt_abort(ioc);
>>> +        if (!want) {
>>> +            free(name);
>>
>> g_free
>>
>>> +        }
>>>            return -1;
>>>        }
>>>        name[namelen] = '\0';
>>>        len -= namelen;
>>> -    if (nbd_drop(ioc, len, errp) < 0) {
>>> +    if (!want) {
>>> +        assert(description);
>>
>> NBD_MAX_NAME_SIZE
> 
> The description is not bound by NBD_MAX_NAME_SIZE.  The NBD protocol DOES 
> document a maximum string size (4k), but we are (so far) not actually 
> honoring that limit (our choice for NBD_MAX_NAME_SIZE is 256, which is 
> smaller than the NBD protocol limit).
> 

Ohm, yes, you are right :(. Too much reviewing, I should stop and make some 
patches :)

>>
>>> +        *description = g_new(char, len + 1);
>>> +        if (nbd_read(ioc, *description, len, errp) < 0) {
>>> +            error_prepend(errp, "failed to read export description: ");
>>> +            nbd_send_opt_abort(ioc);
>>> +            free(name);
>>> +            free(*description);
>>
>> g_free
>>
>>> +            return -1;
>>> +        }
>>> +        (*description)[len] = '\0';
>>> +    } else if (nbd_drop(ioc, len, errp) < 0) {
>>>            error_prepend(errp, "failed to read export description: ");
>>>            nbd_send_opt_abort(ioc);
>>>            return -1;
>>>        }
>>> -    if (!strcmp(name, want)) {
>>> +    if (want && !strcmp(name, want)) {
>>>            *match = true;
>>>        }
>>>        return 1;
>>
>> one more thing: on fail path, you finally fill output name and description
>> with freed pointers. I'd prefer to keep them unchanged in this case, however,
>> it's a matter of taste.
> 
> Okay, I'll try and be more careful in v2 about not altering the callers 
> pointers on failure.
> 


-- 
Best regards,
Vladimir