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 <ebl...@redhat.com>
>>>
>>> ---
>>> 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
