[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#55952: [PATCH] bindat (strz): Write null terminator after variable l
From: |
Richard Hansen |
Subject: |
bug#55952: [PATCH] bindat (strz): Write null terminator after variable length string |
Date: |
Wed, 15 Jun 2022 14:49:58 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 |
Thanks, but AFAICT the documentation doesn't describe accurately
enough what the modified code does: what if the pre-allocated
destination string doesn't have enough storage for the null byte the
code adds?
The existing code advances the index for the terminator, it just
doesn't write 0 to that byte. So the existing code already signals
an error in that case unless the `strz` is the final field.
I don't see how this is relevant to the concern I expressed.
The point I was trying to make is that this patch doesn't change the behavior
(in any significant way) in the case of an undersized output string. Perhaps
the documentation could be improved, but I'd rather do that in a follow-up
patch because it is outside of the scope of this patch.
My concern is that you found it necessary to add a comment about
writing the terminating null byte (which is a good thing), but didn't
mention that aspect in the manual, not even as a hint. I think it is
noteworthy enough to be in the manual.
What do you mean? The patch changes the manual to say:
When packing, the entire input string is copied tothe packed
output followed by a null (zero) byte.
The attached revision tweaks the wording to make it stand out more:
When packing, the entire input string is copied tothe packed
output, and a null (zero) byte is written after that.
Regardless, the documentation for `bindat-pack` [1] clearly states
that the pre-allocated string must have enough room:
When pre-allocating, you should make sure `(length raw)` meets or
exceeds the total length to avoid an out-of-range error.
[1]
https://www.gnu.org/software/emacs/manual/html_node/elisp/Bindat-Functions.html#index-bindat_002dpack
This comes _after_ the place where strz is described, so if someone
reads the manual in order, they wouldn't have read that yet. And even
if they did, there's no reason to assume they remember it well enough.
Bottom line: I think this aspect of the code is important to mention
in the manual. The price is small, whereas the benefit could be
significant.
I disagree -- I think the price is relatively high compared to the benefit. The
pre-allocated length requirement is a requirement of only `bindat-pack` -- not
of `bindat-type` or any of the type specifiers -- so it is best to keep that
requirement with the documentation of `bindat-pack`. Indeed, users are unaware
that packing to a pre-allocated string is even possible until they read the
documentation for `bindat-pack` (except for references in the caveats
documented for fixed-length `str` and `strz`, which I plan on addressing in a
future patch).
Thanks,
Richard
v2-0001-bindat-strz-Write-null-terminator-after-variable-.patch
Description: Text Data
OpenPGP_signature
Description: OpenPGP digital signature