Re: [Qemu-devel] [PATCH v5 05/11] nbd/server: Refactor zero-length option check

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4995 bytes --]

On 10/20/2017 03:34 AM, Vladimir Sementsov-Ogievskiy wrote:
> 20.10.2017 01:26, Eric Blake wrote:
>> Consolidate the check for a zero-length payload to an option
>> into a new function, nbd_check_zero_length(); this check will
>> also be used when introducing support for structured replies.
>>
>> By sticking a catch-all check at the end of the loop for
>> processing options, we can simplify several of the intermediate
>> cases.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> looks like two patches in one, however I'm not against (considering my
> big patches =)).
> I've already put an r-b here but suddenly understood a hidden behavior
> change you've made,
> which may considered like a bug, see below.
> 

>> +/* nbd_check_zero_length: Handle any unexpected payload.
>> + * Return:
>> + * -errno  on error, errp is set
>> + * 0       on successful negotiation, errp is not set
>> + */
>> +static int nbd_check_zero_length(NBDClient *client, uint32_t length,
>> +                                 uint32_t option, Error **errp)
>> +{
>> +    if (!length) {
>> +        return 0;
>> +    }
>> +    if (nbd_drop(client->ioc, length, errp) < 0) {
>> +        return -EIO;
>> +    }
>> +    return nbd_negotiate_send_rep_err(client->ioc,
>> NBD_REP_ERR_INVALID, option,
>> +                                      errp, "option %s should have
>> zero length",
> 
> may be quotes around %s or your trace-notation %d (%s) would be more
> readable

quotes don't hurt, but since none of the option names contain spaces,
it's not quite as important as when you are quoting a message sent over
the wire.

> 
>> +                                      nbd_opt_lookup(option));
>> +}
>> +
>>   /* nbd_negotiate_options
>>    * Process all NBD_OPT_* client option commands, during fixed newstyle
>>    * negotiation.
>> @@ -674,7 +672,11 @@ static int nbd_negotiate_options(NBDClient
>> *client, uint16_t myflags,
>>               }
>>               switch (option) {
>>               case NBD_OPT_STARTTLS:
>> -                tioc = nbd_negotiate_handle_starttls(client, length,
>> errp);
>> +                ret = nbd_check_zero_length(client, length, option,
>> errp);
>> +                if (ret < 0) {
>> +                    return ret;
>> +                }
> 
> no, you should not continue if length>0 (old behavior).
> nbd_negotiate_send_rep_err returns 0 on success
> in nbd_check_zero_length().

Oh, good catch. But it's subtler than that. In the old code,
nbd_negotiate_handle_starttls() returns NULL on non-zero length (even if
it sent a message to the client), because we really want to kill the
connection if a client can't turn on TLS correctly...

>> @@ -712,9 +711,9 @@ static int nbd_negotiate_options(NBDClient
>> *client, uint16_t myflags,
>>           } else if (fixedNewstyle) {
>>               switch (option) {
>>               case NBD_OPT_LIST:
>> -                ret = nbd_negotiate_handle_list(client, length, errp);
>> -                if (ret < 0) {
>> -                    return ret;
>> +                ret = nbd_check_zero_length(client, length, option,
>> errp);
>> +                if (!ret) {
> 
> the same here
> 

while nbd_negotiate_handle_list() used to return 0 if the client sent
non-zero length (we handled the incorrect message from the client just
fine, and can continue listening for more options).

Maybe I can fix it with a tri-state return: 1 if correct length, 0 if
nonzero length but error message sent successfully, and negative on
transmission failure; although then it's trickier for callers.  I'll
have to think about it...

>>               case NBD_OPT_STARTTLS:
>> -                if (nbd_drop(client->ioc, length, errp) < 0) {
>> -                    return -EIO;
>> -                }
>> -                if (client->tlscreds) {
>> +                if (length) {
>> +                    ret = nbd_check_zero_length(client, length,
>> option, errp);

Maybe explicitly checking for length at each caller is the simplest
approach for getting the decision logic correct, since I really wasn't
able to abstract a clean "failure to communicate" vs. "error message
sent, go on to next message or abort connection as appropriate" vs.
"everything validated, proceed with rest of handing current option".

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]