Re: [Qemu-devel] [PATCH v2 12/22] nbd/client: Improve error handling in nbd_negotiate_simple_meta_context()

[Vladimir Sementsov-Ogievskiy]

17.12.2018 18:30, Eric Blake wrote:
> On 12/15/18 9:19 AM, Richard W.M. Jones wrote:
>> On Sat, Dec 15, 2018 at 07:53:14AM -0600, Eric Blake wrote:
>>> Always allocate space for the reply returned by the server and
>>> hoist the trace earlier, as it is more interesting to trace the
>>> server's reply (even if it is unexpected) than parroting our
>>> request only on success.  After all, skipping the allocation
>>> for a wrong size was merely a micro-optimization that only
>>> benefitted a broken server, rather than the common case of a
>>> compliant server that meets our expectations.
>>>
>>> Then turn the reply handling into a loop (even though we still
>>> never iterate more than once), to make this code easier to use
>>> when later patches do support multiple server replies.  This
>>> changes the error message for a server with two replies (a
>>> corner case we are unlikely to hit in practice) from:
>>>
>>> Unexpected reply type 4 (meta context), expected 0 (ack)
>>>
>>> to:
>>>
>>> Server replied with more than one context
>>>
>>> Signed-off-by: Eric Blake <address@hidden>
>>>
>>> ---
>>> v2: split patch into easier-to-review pieces [Rich, Vladimir]
>>> ---
>>>   nbd/client.c | 16 ++++++++++++----
>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/nbd/client.c b/nbd/client.c
>>> index bcccd5f555e..b6a85fc3ef8 100644
>>> --- a/nbd/client.c
>>> +++ b/nbd/client.c
>>> @@ -684,10 +684,11 @@ static int 
>>> nbd_negotiate_simple_meta_context(QIOChannel *ioc,
>>>           return ret;
>>>       }
>>>
>>> -    if (reply.type == NBD_REP_META_CONTEXT) {
>>> +    while (reply.type == NBD_REP_META_CONTEXT) {
>>
>> I'm not sure I understand why this change is safe.
>>
>> As far as I can see reply.type is only updated in the loop by
>> nbd_receive_option_reply, and that reads from the server, and so the
>> server might keep sending NBD_REP_META_CONTEXT packets (instead of the
>> expected NBD_REP_ACK), so it could now loop forever against a
>> malicious server?  (This is not taking into account any later patches)
> 
> Hmm - now that I've already responded to why the conversion to a loop does 
> not change this code, I'm now wondering if I even need this patch. In v1 of 
> the series, both SET and LIST shared a common function, and since LIST needs 
> the loop, converting SET to use a loop that exits early if it executes more 
> than once was needed to make the two actions share a common entry point.  But 
> since v2 uses different entry points (because it separated the common code 
> into separate helper functions, leaving the SET entry point unchanged and 
> adding a new LIST entry point), where only the LIST entry point actually has 
> to loop, I might be able to just drop this patch entirely and still achieve 
> the same effect.
> 

Are you going to resend series without this patch? In this case I'd prefer to 
continue review on already rebased patches. Or it doesn't make much difference?

-- 
Best regards,
Vladimir