Re: [Nbd] [RFC PATCH] doc: In STRUCTURED_REPLY, make error types easy to recognize

[Wouter Verhelst <w@...112...>]

On Sat, Apr 09, 2016 at 11:17:50AM +0100, Alex Bligh wrote:
> > 
> > So, let's use two examples, of two different NBD_REPLY_TYPE_ERROR_OFFSET
> > messages (one with string, the other without). As I originally wrote the
> > RFC, they would be sent over the wire as:
> > 
> > With my original RFC, the same two messages were:
> > 
> > 4 bytes: error
> > 4 bytes: message length (11)
> > 8 bytes: offset
> > 11 bytes: "hello world"
> > 
> > 4 bytes: error
> > 4 bytes: message length (0)
> > 8 bytes: offset
> > 
> > Under your counter of keeping message length and contents next to one
> > another, plus my idea above of only 2 bytes for message length because
> > of our 4096 string length cap, those same to chunks become:
> > 
> > 4 bytes: error
> > 2 bytes: message length (11)
> > 11 bytes: "hello world"
> > 8 bytes: offset
> > 
> > 4 bytes: error
> > 2 bytes: message length (0)
> > 8 bytes: offset
> 
> yes

It's always easier to add new data at the end rather than in the middle.
With the former, you can just use a struct to read data off the wire,
and it won't change because someone changed the message. With the
latter, that isn't the case.

(this is also pretty much how ASN.1 works, IIUC)

> > And looking at it, I see another viable alternative:
> > 
> > 32 bits: error (non-zero)
> > arbitrary (up to 4096) number of bytes: message string, no NUL bytes
> > 1-4 bytes: NUL padding (to be nice, server SHOULD round to
> >   boundary of 4, but client MUST be prepared for any alignment
> >   on remaining fields)
> > remaining bytes: additional structure, size determined by reply
> >   header length
> > 
> > That way, we've guaranteed that the error string has NUL termination
> > (unlike in other places), and no longer need to advertise the message
> > length over the wire. We've also guaranteed that the message string
> > starts at a fixed offset (the client can ALWAYS dump whatever string it
> > finds at the offset immediately following the error field, and dump
> > until it hits a NUL byte, even if the client has no idea how to parse
> > the tail of the message).  And clients that DO recognize the error type
> > can take header_length - lookup_table[reply_type] as the offset where
> > the important binary fields live in the remainder of the reply.
> > 
> > Under this alternate, two same two examples are sent over the wire as:
> > 
> > 4 bytes: error
> > 11 bytes: "hello world"
> > 1 byte: pad: {'\0'}
> > 8 bytes: offset
> > 
> > 4 bytes: error
> > 0 bytes: ""
> > 4 bytes: pad: {'\0', '\0', '\0', '\0'}
> > 8 bytes: offset
> > 
> > Preferences?
> > 
> 
> Let's not do this. This is going to make people think strings are
> nul terminated as even though they aren't, quite, nul termination
> will work becuas of the padding nul. Also if the client 'MUST' be prepared
> for any alignment, how does it know what the padding bytes are?
> 
> Everywhere else in NBD, strings are simple strings with no padding,
> and no NUL after them, and are immediately preceded by their length.

I explicitly did not add NUL bytes because adding them would allow an
evil server to send a string without a NUL byte (violating the protocol)
followed by a stack smashing attack (or some such), and hack into a
client.

Never a good idea to assume that a server is not evil.

So no, I don't want to see explicit terminating NUL bytes in the
protocol, either.

[...]

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12