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

Previous Next

Full log

View this message in rfc822 format

From: help-debbugs <at> gnu.org (GNU bug Tracking System)
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: tracker <at> debbugs.gnu.org
Subject: bug#55952: closed ([PATCH] bindat (strz): Write null terminator
 after variable length string)
Date: Thu, 16 Jun 2022 07:16:01 +0000

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

Your message dated Thu, 16 Jun 2022 10:15:40 +0300
with message-id <83fsk5qdsz.fsf <at> gnu.org>
and subject line Re: bug#55952: [PATCH] bindat (strz): Write null terminator after variable length string
has caused the debbugs.gnu.org bug report #55952,
regarding [PATCH] bindat (strz): Write null terminator after variable length string
to be marked as done.

(If you believe you have received this mail in error, please contact
help-debbugs <at> gnu.org.)


-- 
55952: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=55952
GNU Bug Tracking System
Contact help-debbugs <at> gnu.org with problems

[Message part 2 (message/rfc822, inline)]

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 3 (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 part 5 (message/rfc822, inline)]

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.