[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] bindat strz fixes
From: |
Richard Hansen |
Subject: |
Re: [PATCH] bindat strz fixes |
Date: |
Wed, 1 Jun 2022 16:23:57 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 |
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