Eric, Merged. Alex On 20 Apr 2016, at 13:15, Eric Blake <eblake@...696...> wrote: > On 04/20/2016 04:33 AM, Alex Bligh wrote: >> Eric, >> >> Thanks for breaking it up. >> > >> >> By embedding string lengths inside the data >> you've made the existing types inherently expandable, which is good. > > Intentionally only the error types, and there, the length is ONLY > expandable by the error message, not by anything else. Rather than > expanding existing types, we should create new types any time we want to > return additional information. > >> But you need to make this clearer. I would add "The client MUST skip any data >> within the *length* field of the reply header which is unused >> by the specified reply types below; this area is reserved for >> expansion." (or similar) > > Not for regular chunked replies, and for error replies, I did that. But > I can tweak the wording, if it helps. > > >>> + >>> +- `NBD_REPLY_TYPE_ERROR` (2^15 + 1) >>> + >>> + This chunk type is in the error chunk category. *length* MUST >>> + be at least 6. This chunk represents that an error occurred, >>> + and the client MAY NOT make any assumptions about partial >> >> that should be 'MUST NOT' (existing problem). > > Okay. > >> >>> + success. This type SHOULD NOT be used more than once in a >>> + structured reply. Valid as a reply to any request. >>> + >>> + The payload is structured as: >>> + >>> + 32 bits: error (MUST be nonzero) >>> + 16 bits: message length (no more than header *length* - 6) >>> + *message length* bytes: optional string suitable for >>> + direct display to a human being >> >> Everywhere else where we have a string we have a 32 byte message >> length. For the sake of consistency, given this is hardly >> speed criticial, perhaps we could stick with that. > > But strings MUST NOT exceed 4096 bytes, which means 32 bits is wasting 2 > bytes over the wire. I see no reason to make it easier for a rogue > server to do a denial of service attack on a guest by making it possible > to pass a length larger than 64k when 4096 is already the upper limit a > client is expecting. > > >>> + 32 bits: error (MUST be non-zero) >>> + 16 bits: message length (no more than header *length* - 14) >>> + *message length* bytes: optional string suitable for >>> + direct display to a human being >> >> Same point re 16/32 bit string lengths >> >>> + 64 bits: offset (unsigned) >> >> Perhaps put the offset before the message? I'm just thinking >> about the implementor who would probably read things into >> a struct if they have the chance. > > No. The implementor KNOWS that a message is at offset 6 within the > message, and that the string length is at offset 4, without knowing the > interpretation of the rest of the payload. So the rest of the payload > MUST come after the message, in order for this scheme of the message > always being at a known location to work. > >> >> That's how it was before, and you seem to have reordered (not >> sure why). > > Because of our earlier conversation on the same topic: > > https://sourceforge.net/p/nbd/mailman/nbd-general/thread/20160409104438.GP19023%40grep.be/#msg35003527 > > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > -- Alex Bligh
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail