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

[Eli Zaretskii]

> Date: Tue, 14 Jun 2022 16:47:47 -0400
> Cc: 55952@debbugs.gnu.org, monnier@iro.umontreal.ca
> From: Richard Hansen <rhansen@rhansen.org>
> 
> > 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.

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.

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

Thanks.