[v6,06/12] nbd/server: Refactor zero-length option check

Commit Message

Eric Blake Oct. 30, 2017, 8:11 p.m. UTC

On 10/30/2017 06:22 PM, Vladimir Sementsov-Ogievskiy wrote:
> 27.10.2017 13:40, Eric Blake wrote:
>> Consolidate the response for a non-zero-length option payload
>> into a new function, nbd_reject_length().  This check will
>> also be used when introducing support for structured replies.
>>
>> Note that STARTTLS response differs based on time: if the connection
>> is still unencrypted, we set fatal to true (a client that can't
>> request TLS correctly may still think that we are ready to start
>> the TLS handshake, so we must disconnect); while if the connection
>> is already encrypted, the client is sending a bogus request but
>> is no longer at risk of being confused by continuing the connection.
>>

>>               switch (option) {
>>               case NBD_OPT_STARTTLS:
>> -                tioc = nbd_negotiate_handle_starttls(client, length,
>> errp);
>> +                if (length) {
>> +                    /* Unconditionally drop the connection if the client
>> +                     * can't start a TLS negotiation correctly */
>> +                    nbd_reject_length(client, length, option, true,
>> errp);
>> +                    return -EINVAL;
> 
> why not to return nbd_reject_length's result? this EINVAL may not
> correspond to errp (about EIO for example)..

Somewhat true, if nbd_reject_length() fails. But nbd_reject_length() may
also return 0 without setting errp, in which case, maybe this code
should set errp rather than just blindly returning -EINVAL.

> 
> with or without this fixed:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> 

Okay, I'll squash this in, and include it in my pull request to be sent
shortly.

                 }
                 tioc = nbd_negotiate_handle_starttls(client, errp);
                 if (!tioc) {

Comments

Eric Blake Oct. 30, 2017, 8:46 p.m. UTC | #1

On 10/30/2017 09:11 PM, Eric Blake wrote:
> On 10/30/2017 06:22 PM, Vladimir Sementsov-Ogievskiy wrote:
>> 27.10.2017 13:40, Eric Blake wrote:
>>> Consolidate the response for a non-zero-length option payload
>>> into a new function, nbd_reject_length().  This check will
>>> also be used when introducing support for structured replies.
>>>

>>> +                if (length) {
>>> +                    /* Unconditionally drop the connection if the client
>>> +                     * can't start a TLS negotiation correctly */
>>> +                    nbd_reject_length(client, length, option, true,
>>> errp);
>>> +                    return -EINVAL;
>>
>> why not to return nbd_reject_length's result? this EINVAL may not
>> correspond to errp (about EIO for example)..
> 
> Somewhat true, if nbd_reject_length() fails. But nbd_reject_length() may
> also return 0 without setting errp, in which case, maybe this code
> should set errp rather than just blindly returning -EINVAL.
> 
>>
>> with or without this fixed:
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
>>
> 
> Okay, I'll squash this in, and include it in my pull request to be sent
> shortly.

D'oh. Long week for me. The whole reason I added a 'bool fatal'
parameter was so that I don't have to worry about nbd_reject_length()
returning 0.  So it is instead better to just do:

> --- i/nbd/server.c
> +++ w/nbd/server.c
> @@ -684,8 +684,13 @@ static int nbd_negotiate_options(NBDClient *client,
> uint16_t myflags,
>                  if (length) {
>                      /* Unconditionally drop the connection if the client
>                       * can't start a TLS negotiation correctly */
> -                    nbd_reject_length(client, length, option, true, errp);
> -                    return -EINVAL;
> +                    return nbd_reject_length(client, length, option, true,
> +                                             errp);

rather than repeating an error message.

Patch

diff --git i/nbd/server.c w/nbd/server.c
index a9480e42cd..91f81a0f19 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -684,8 +684,13 @@  static int nbd_negotiate_options(NBDClient *client,
uint16_t myflags,
                 if (length) {
                     /* Unconditionally drop the connection if the client
                      * can't start a TLS negotiation correctly */
-                    nbd_reject_length(client, length, option, true, errp);
-                    return -EINVAL;
+                    ret = nbd_reject_length(client, length, option,
true, errp);
+                    if (!ret) {
+                        error_setg(errp, "option '%s' should have zero
length",
+                                   nbd_opt_lookup(option));
+                        ret = -EINVAL;
+                    }
+                    return ret;