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

From: Richard Hansen <[email-masked]>
Date: Fri, 17 Jun 2022 23:02:57 -0400

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 <[email-masked]>
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 <[email-masked]>
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")
-- 
2.36.1