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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 55952 in the body.
You can then email your comments to 55952 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Richard Hansen <rhansen <at> rhansen.org>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] bindat (strz): Write null terminator after variable length
 string
Date: Mon, 13 Jun 2022 17:48:15 -0400

[Message part 1 (text/plain, inline)]

X-Debbugs-CC: monnier <at> iro.umontreal.ca

Attached patch:

* lisp/emacs-lisp/bindat.el (bindat--pack-strz): Explicitly write a
null byte after packing a variable-length string to ensure proper
termination when packing to a pre-allocated string.
* doc/lispref/processes.texi (Bindat Types): Update documentation.
* test/lisp/emacs-lisp/bindat-tests.el (bindat-test--str-strz-prealloc):
Update tests.

[0001-bindat-strz-Write-null-terminator-after-variable-len.patch (text/x-patch, attachment)]

Message #8 received at 55952 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Richard Hansen <rhansen <at> rhansen.org>
Cc: 55952 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#55952: [PATCH] bindat (strz): Write null terminator after
 variable length string
Date: Tue, 14 Jun 2022 15:52:02 +0300

> Cc: monnier <at> iro.umontreal.ca
> Date: Mon, 13 Jun 2022 17:48:15 -0400
> From: Richard Hansen <rhansen <at> rhansen.org>
> 
> Attached patch:
> 
> * lisp/emacs-lisp/bindat.el (bindat--pack-strz): Explicitly write a
> null byte after packing a variable-length string to ensure proper
> termination when packing to a pre-allocated string.
> * doc/lispref/processes.texi (Bindat Types): Update documentation.
> * test/lisp/emacs-lisp/bindat-tests.el (bindat-test--str-strz-prealloc):
> Update tests.

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?

Message #11 received at 55952 <at> debbugs.gnu.org (full text, mbox):

From: Richard Hansen <rhansen <at> rhansen.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 55952 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#55952: [PATCH] bindat (strz): Write null terminator after
 variable length string
Date: Tue, 14 Jun 2022 16:47:47 -0400

[Message part 1 (text/plain, inline)]

On 6/14/22 08:52, Eli Zaretskii wrote:
> 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.

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

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #14 received at 55952 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Richard Hansen <rhansen <at> rhansen.org>
Cc: 55952 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#55952: [PATCH] bindat (strz): Write null terminator after
 variable length string
Date: Wed, 15 Jun 2022 15:16:38 +0300

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

Message #17 received at 55952 <at> debbugs.gnu.org (full text, mbox):

From: Richard Hansen <rhansen <at> rhansen.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 55952 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#55952: [PATCH] bindat (strz): Write null terminator after
 variable length string
Date: Wed, 15 Jun 2022 14:49:58 -0400

[Message part 1 (text/plain, inline)]

>>> 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 (text/x-patch, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #22 received at 55952-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Richard Hansen <rhansen <at> rhansen.org>
Cc: monnier <at> iro.umontreal.ca, 55952-done <at> debbugs.gnu.org
Subject: Re: bug#55952: [PATCH] bindat (strz): Write null terminator after
 variable length string
Date: Thu, 16 Jun 2022 10:15:40 +0300

> Date: Wed, 15 Jun 2022 14:49:58 -0400
> Cc: 55952 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> From: Richard Hansen <rhansen <at> rhansen.org>
> 
> >> 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.

I never had any objections to changing the behavior, nor in general to
the code changes in your patch.  My comments were only about the
documentation in the manual.

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

I meant that the caveat about having enough space in the output string
for the terminating null byte is not explicitly mentioned where strz
and its handling during packing are documented.

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

I don't see any point in continuing to argue about this.  I have my
opinions about how our manuals should explain these issues, and your
disagreement, which is noted, doesn't change those opinions.  So I
installed your previous version, and then made the changes in the
manual I thought were pertinent.

Thanks.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.