Re: [PATCH] bindat strz fixes

[Richard Hansen]

On 6/1/22 08:04, Stefan Monnier wrote:
> >    * The documentation says that the packed output is null terminated
> >      so that's what users expect.
> >    * It is safer (packed output is less likely to cause some other
> >      program to run past the end of the field).
> >    * Without this change, there is no difference between `strz N` and
> >      `str N`. So what would be the point of `strz N`?
> >    * If the user selected strz, the application probably requires null
> >      termination in all cases, not just when the string is under a
> >      certain length.
>
> You're listing advantages of this choice, which we know about already.
> The other choice also has advantages.  These don't count as "particular
> reasons" (e.g. a real-life concrete *need* for it, rather than a mere
> preference).

I don't have a concrete need for it at the moment. I just noticed the bug while 
I was fixing the other bugs. I can leave that change out of the patch series if 
that is the only thing impeding merge.

> The particular reason to prefer the current behavior is
> backward compatibility (which we could call "inertia").

Anyone that needs the current behavior should be using `str N`, not `strz N`. 
If they're using `strz N`, then I would consider that to be a bug in their 
code. If this change breaks someone, they can fix it easily: just change `strz 
N` to `str N`. I understand that we should endeavor to maintain compatibility, 
but keeping the current behavior would be intentionally preserving a bug to 
accommodate other bugs. I don't think that's a good trade-off.

> Note also that `strz` without a length (or with a nil length) behaves
> the way you want.

No -- strz without a length results in variable length encoding. Without these 
changes, the only way to get a fixed length output with guaranteed null 
termination is to do `(substring str 0 (1- N))` before packing. (Or explicitly 
pack a separate zero byte immediately after the `str N-1` or `strz N-1` entry.)

> Of course, we could add an additional (optional) arg to `strz` to
> specify what should happen when unpacking a string with missing NUL byte
> as well as whether to truncate to N-1 chars rather than to N chars to
> make sure there is a terminating NUL byte.

I don't think that's necessary. I think most use cases are satisfied by the 
current str behavior and the strz behavior with these patches.

> > > How 'bout
> > >        (bindat-unpack spec "ab")
> > > ?
> >
> > I added some comments explaining why cases like that aren't tested.
>
> The byte-string to unpack is not necessarily built from our own code
> (usually bindat is used to communicate with some other application), so
> whether our code can generate "ab" is not really relevant: the question
> still comes up about what we should do with "ab" (where valid answers
> could be to return "ab" or to return "a" or to signal an error, ...).
> Of course we can also decide it's "undefined".

Regardless of what other applications produce, packed strings like that are 
invalid according to the spec provided to `bindat-unpack`. Attempting to do 
something useful with such invalid strings assumes that Postel's Law is a good 
thing, which I don't agree with (especially with security-sensitive protocols). 
I think it is safest to signal an error, which it currently does.

A real example of why it might be undesirable to attempt to process a strz 
string without a null terminator:

  1. The sender streams multiple packed messages over a reliable, in-order 
protocol like TCP or pipes.
  2. The sending system breaks up the messages at arbitrary places to fit in 
packets/buffers, so the receiving side might see an incomplete message that 
will be completed by a future packet (or multiple future packets).
  3. The receiver attempts to extract a message from the bytes received so far. 
If the bytes do not form a valid message according to the bindat-unpack spec, 
then assume the message was truncated. Remember the bytes received so far and 
wait for the next packet.
  4. When the next packet arrives, concatenate its bytes with the remembered 
bytes and try again.

If bindat-unpack did not signal an error when expecting a null terminator, then 
the above protocol would not work without extra effort by the user.

Either way, changing the way `bindat-unpack` handles invalid strings feels out 
of scope for this particular bug report.

OpenPGP_signature
Description: OpenPGP digital signature