Package: emacs;
Reported by: Richard Hansen <rhansen <at> rhansen.org>
Date: Fri, 17 Jun 2022 22:54:02 UTC
Severity: wishlist
Tags: patch
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Richard Hansen <rhansen <at> rhansen.org> Cc: 56048 <at> debbugs.gnu.org Subject: bug#56048: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room Date: Tue, 21 Jun 2022 16:44:06 -0400
Thanks, Richard. Looks good to me.
Stefan
Richard Hansen [2022-06-17 23:02:57] wrote:
> Attached are new revisions of the patches. The only differences are the
> comments were filled at column 70 instead of 80, and the commit message
> mentions the bug number.
>
> From 6096bc8bceada87a5c54e4eb500812a0b72ffd44 Mon Sep 17 00:00:00 2001
> From: Richard Hansen <rhansen <at> rhansen.org>
> Date: Sun, 29 May 2022 21:23:57 -0400
> Subject: [PATCH v2 1/2] ; bindat (strz): Move all pack logic to pack function
>
> ---
> lisp/emacs-lisp/bindat.el | 49 ++++++++++++++++++---------------------
> 1 file changed, 23 insertions(+), 26 deletions(-)
>
> diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
> index 46e2a4901c..4a642bb9c5 100644
> --- a/lisp/emacs-lisp/bindat.el
> +++ b/lisp/emacs-lisp/bindat.el
> @@ -440,20 +440,27 @@ bindat--pack-str
> (aset bindat-raw (+ bindat-idx i) (aref v i)))
> (setq bindat-idx (+ bindat-idx len))))
>
> -(defun bindat--pack-strz (v)
> +(defun bindat--pack-strz (len v)
> (let* ((v (string-to-unibyte v))
> - (len (length v)))
> - (dotimes (i len)
> - (when (= (aref v i) 0)
> - ;; Alternatively we could pretend that this was the end of
> - ;; the string and stop packing, but then bindat-length would
> - ;; need to scan the input string looking for a null byte.
> - (error "Null byte encountered in input strz string"))
> - (aset bindat-raw (+ bindat-idx i) (aref v i)))
> - ;; Explicitly write a null terminator in case the user provided a
> - ;; pre-allocated string to bindat-pack that wasn't zeroed first.
> - (aset bindat-raw (+ bindat-idx len) 0)
> - (setq bindat-idx (+ bindat-idx len 1))))
> + (vlen (length v)))
> + (if len
> + ;; When len is specified, behave the same as the str type
> + ;; since we don't actually add the terminating zero anyway
> + ;; (because we rely on the fact that `bindat-raw' was
> + ;; presumably initialized with all-zeroes before we started).
> + (bindat--pack-str len v)
> + (dotimes (i vlen)
> + (when (= (aref v i) 0)
> + ;; Alternatively we could pretend that this was the end of
> + ;; the string and stop packing, but then bindat-length would
> + ;; need to scan the input string looking for a null byte.
> + (error "Null byte encountered in input strz string"))
> + (aset bindat-raw (+ bindat-idx i) (aref v i)))
> + ;; Explicitly write a null terminator in case the user provided
> + ;; a pre-allocated string to `bindat-pack' that wasn't already
> + ;; zeroed.
> + (aset bindat-raw (+ bindat-idx vlen) 0)
> + (setq bindat-idx (+ bindat-idx vlen 1)))))
>
> (defun bindat--pack-bits (len v)
> (let ((bnum (1- (* 8 len))) j m)
> @@ -482,7 +489,8 @@ bindat--pack-item
> ('u24r (bindat--pack-u24r v))
> ('u32r (bindat--pack-u32r v))
> ('bits (bindat--pack-bits len v))
> - ((or 'str 'strz) (bindat--pack-str len v))
> + ('str (bindat--pack-str len v))
> + ('strz (bindat--pack-strz len v))
> ('vec
> (let ((l (length v)) (vlen 1))
> (if (consp vectype)
> @@ -699,18 +707,7 @@ bindat--type
> ((numberp len) len)
> ;; General expression support.
> (t `(or ,len (1+ (length ,val)))))))
> - (`(pack . ,args)
> - ;; When len is specified, behave the same as the str type since we don't
> - ;; actually add the terminating zero anyway (because we rely on the fact
> - ;; that `bindat-raw' was presumably initialized with all-zeroes before we
> - ;; started).
> - (cond ; Same optimizations as 'length above.
> - ((null len) `(bindat--pack-strz . ,args))
> - ((numberp len) `(bindat--pack-str ,len . ,args))
> - (t (macroexp-let2 nil len len
> - `(if ,len
> - (bindat--pack-str ,len . ,args)
> - (bindat--pack-strz . ,args))))))))
> + (`(pack . ,args) `(bindat--pack-strz ,len . ,args))))
>
> (cl-defmethod bindat--type (op (_ (eql 'bits)) len)
> (bindat--pcase op
> --
> 2.36.1
>
>
> From 9ebda68264adca6f60f780d44995c4213d2c12c2 Mon Sep 17 00:00:00 2001
> From: Richard Hansen <rhansen <at> rhansen.org>
> Date: Thu, 16 Jun 2022 15:21:57 -0400
> Subject: [PATCH v2 2/2] bindat (strz): Null terminate fixed-length strings if
> there is room
>
> * lisp/emacs-lisp/bindat.el (bindat--pack-strz): For fixed-length strz
> fields, explicitly write a null terminator after the packed string if
> there is room (bug#56048).
> * doc/lispref/processes.texi (Bindat Types): Update documentation.
> * test/lisp/emacs-lisp/bindat-tests.el (bindat-test--str-strz-prealloc):
> Update tests.
> ---
> doc/lispref/processes.texi | 30 ++++++++++++++--------------
> lisp/emacs-lisp/bindat.el | 13 ++++++------
> test/lisp/emacs-lisp/bindat-tests.el | 12 +++++------
> 3 files changed, 27 insertions(+), 28 deletions(-)
>
> diff --git a/doc/lispref/processes.texi b/doc/lispref/processes.texi
> index b9200aedde..d038d52d44 100644
> --- a/doc/lispref/processes.texi
> +++ b/doc/lispref/processes.texi
> @@ -3509,23 +3509,23 @@ Bindat Types
> (but excluding) the null byte that terminated the input string.
>
> If @var{len} is provided, @code{strz} behaves the same as @code{str},
> -but with one difference: when unpacking, the first null byte
> -encountered in the packed string is interpreted as the terminating
> -byte, and it and all subsequent bytes are excluded from the result of
> -the unpacking.
> +but with a couple of differences:
> +
> +@itemize @bullet
> +@item
> +When packing, a null terminator is written after the packed string if
> +the length of the input string is less than @var{len}.
> +
> +@item
> +When unpacking, the first null byte encountered in the packed string
> +is interpreted as the terminating byte, and it and all subsequent
> +bytes are excluded from the result of the unpacking.
> +@end itemize
>
> @quotation Caution
> -The packed output will not be null-terminated unless one of the
> -following is true:
> -@itemize
> -@item
> -The input string is shorter than @var{len} bytes and either no pre-allocated
> -string was provided to @code{bindat-pack} or the appropriate byte in
> -the pre-allocated string was already null.
> -@item
> -The input string contains a null byte within the first @var{len}
> -bytes.
> -@end itemize
> +The packed output will not be null-terminated unless the input string
> +is shorter than @var{len} bytes or it contains a null byte within the
> +first @var{len} bytes.
> @end quotation
>
> @item vec @var{len} [@var{type}]
> diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
> index 4a642bb9c5..0ecac3d52a 100644
> --- a/lisp/emacs-lisp/bindat.el
> +++ b/lisp/emacs-lisp/bindat.el
> @@ -443,11 +443,14 @@ bindat--pack-str
> (defun bindat--pack-strz (len v)
> (let* ((v (string-to-unibyte v))
> (vlen (length v)))
> + ;; Explicitly write a null terminator (if there's room) in case
> + ;; the user provided a pre-allocated string to `bindat-pack' that
> + ;; wasn't already zeroed.
> + (when (or (null len) (< vlen len))
> + (aset bindat-raw (+ bindat-idx vlen) 0))
> (if len
> ;; When len is specified, behave the same as the str type
> - ;; since we don't actually add the terminating zero anyway
> - ;; (because we rely on the fact that `bindat-raw' was
> - ;; presumably initialized with all-zeroes before we started).
> + ;; (except for the null terminator possibly written above).
> (bindat--pack-str len v)
> (dotimes (i vlen)
> (when (= (aref v i) 0)
> @@ -456,10 +459,6 @@ bindat--pack-strz
> ;; need to scan the input string looking for a null byte.
> (error "Null byte encountered in input strz string"))
> (aset bindat-raw (+ bindat-idx i) (aref v i)))
> - ;; Explicitly write a null terminator in case the user provided
> - ;; a pre-allocated string to `bindat-pack' that wasn't already
> - ;; zeroed.
> - (aset bindat-raw (+ bindat-idx vlen) 0)
> (setq bindat-idx (+ bindat-idx vlen 1)))))
>
> (defun bindat--pack-bits (len v)
> diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
> index cc223ad14e..0c03c51e2e 100644
> --- a/test/lisp/emacs-lisp/bindat-tests.el
> +++ b/test/lisp/emacs-lisp/bindat-tests.el
> @@ -172,14 +172,14 @@ bindat-test--str-strz-prealloc
> ((((x str 2)) ((x . "a"))) . "ax")
> ((((x str 2)) ((x . "ab"))) . "ab")
> ((((x str 2)) ((x . "abc"))) . "ab")
> - ((,(bindat-type strz 1) "") . "xx")
> - ((,(bindat-type strz 2) "") . "xx")
> - ((,(bindat-type strz 2) "a") . "ax")
> + ((,(bindat-type strz 1) "") . "\0x")
> + ((,(bindat-type strz 2) "") . "\0x")
> + ((,(bindat-type strz 2) "a") . "a\0")
> ((,(bindat-type strz 2) "ab") . "ab")
> ((,(bindat-type strz 2) "abc") . "ab")
> - ((((x strz 1)) ((x . ""))) . "xx")
> - ((((x strz 2)) ((x . ""))) . "xx")
> - ((((x strz 2)) ((x . "a"))) . "ax")
> + ((((x strz 1)) ((x . ""))) . "\0x")
> + ((((x strz 2)) ((x . ""))) . "\0x")
> + ((((x strz 2)) ((x . "a"))) . "a\0")
> ((((x strz 2)) ((x . "ab"))) . "ab")
> ((((x strz 2)) ((x . "abc"))) . "ab")
> ((,(bindat-type strz) "") . "\0x")
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.