Subject: bug#56048: closed (Re: bug#56048: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room)

From: [email-masked] (GNU bug Tracking System)
Date: Wed, 22 Jun 2022 13:58:02 +0000

Your bug report

#56048: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
56048: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=56048
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#56048: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room
From: Eli Zaretskii <[email-masked]>
To: Stefan Monnier <[email-masked]>
Cc: [email-masked], [email-masked]
Date: Wed, 22 Jun 2022 16:57:19 +0300
List-Id: <debbugs-submit.debbugs.gnu.org>

> Cc: [email-masked]
> Date: Tue, 21 Jun 2022 16:44:06 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <[email-masked]>
> 
> Thanks, Richard.  Looks good to me.

Installed on master.

Subject: [PATCH] bindat (strz): Null terminate fixed-length strings if there is room
From: Richard Hansen <[email-masked]>
To: [email-masked]
Date: Fri, 17 Jun 2022 18:52:51 -0400
List-Id: <debbugs-submit.debbugs.gnu.org>

X-Debbugs-CC: [email-masked]

Two patches attached:

Patch 1:

     ; bindat (strz): Move all pack logic to pack function

Patch 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.
     * doc/lispref/processes.texi (Bindat Types): Update documentation.
     * test/lisp/emacs-lisp/bindat-tests.el (bindat-test--str-strz-prealloc):
     Update tests.
From 062de6aae2e37110ac37557058d1738d009cdd77 Mon Sep 17 00:00:00 2001
From: Richard Hansen <[email-masked]>
Date: Sun, 29 May 2022 21:23:57 -0400
Subject: [PATCH 1/2] ; bindat (strz): Move all pack logic to pack function

---
 lisp/emacs-lisp/bindat.el | 48 ++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index 46e2a4901c..f7f540f5fd 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -440,20 +440,26 @@ 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 zeroed first.
+      (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 +488,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 +706,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 325100cd7da32e9a13bc94a544896b69824aa12e Mon Sep 17 00:00:00 2001
From: Richard Hansen <[email-masked]>
Date: Thu, 16 Jun 2022 15:21:57 -0400
Subject: [PATCH 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.
* 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            | 14 ++++++-------
 test/lisp/emacs-lisp/bindat-tests.el | 12 +++++------
 3 files changed, 28 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 f7f540f5fd..6428b0e965 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).
+        ;; When len is specified, behave the same as the str type (except for
+        ;; the null terminator possibly written above).
         (bindat--pack-str len v)
       (dotimes (i vlen)
         (when (= (aref v i) 0)
@@ -456,9 +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 zeroed first.
-      (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")
-- 
2.36.1