bug#55952: [PATCH] bindat (strz): Write null terminator after variable length string

[Richard Hansen]

> > > 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