--- Begin Message ---
|
Subject: |
[PATCH] bindat (strz): Write null terminator after variable length string |
|
Date: |
Mon, 13 Jun 2022 17:48:15 -0400 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 |
X-Debbugs-CC: monnier@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
Description: Text Data
--- End Message ---
--- Begin Message ---
|
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@debbugs.gnu.org, monnier@iro.umontreal.ca
> From: Richard Hansen <rhansen@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.
--- End Message ---