bug#55719: [PATCH] bindat strz fixes

bug#55719: [PATCH] bindat strz fixes

[Richard Hansen]

[-- Attachment #1.1.1: Type: text/plain, Size: 190 bytes --]

The attached series of patches should fix bug #55719 (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=55719).

I have already signed the copyright assignment agreement.

Thanks,
Richard

[-- Attachment #1.1.2: 0001-bindat-tests-strz-Add-more-tests.patch --]
[-- Type: text/x-patch, Size: 3175 bytes --]

From 92223dc60acc6531ac86bfcee1eebc38f8304841 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 29 May 2022 17:15:04 -0400
Subject: [PATCH 1/7] ; bindat-tests (strz): Add more tests

---
 test/lisp/emacs-lisp/bindat-tests.el | 60 ++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
index 7722cf6c02..53c0c359d8 100644
--- a/test/lisp/emacs-lisp/bindat-tests.el
+++ b/test/lisp/emacs-lisp/bindat-tests.el
@@ -162,4 +162,64 @@ bindat-test--recursive
                                         (bindat-pack bindat-test--LEB128 n))
                          n)))))))
 
+(let ((spec (bindat-type :pack-var v
+                         (x strz 2 :pack-val v)
+                         :unpack-val x)))
+  (ert-deftest bindat-test--strz-fixedlen-len ()
+    (should (equal (bindat-length spec "") 2))
+    (should (equal (bindat-length spec "a") 2)))
+
+  (ert-deftest bindat-test--strz-fixedlen-len-overflow ()
+    (should (equal (bindat-length spec "abc") 2)))
+
+  (ert-deftest bindat-test--strz-fixedlen-pack ()
+    (should (equal (bindat-pack spec "") "\0\0"))
+    (should (equal (bindat-pack spec "a") "\141\0")))
+
+  (ert-deftest bindat-test--strz-fixedlen-pack-overflow ()
+    :expected-result :failed
+    (should (equal (bindat-pack spec "abc") "\141\0")))
+
+  (ert-deftest bindat-test--strz-fixedlen-unpack ()
+    (should (equal (bindat-unpack spec "\0\0") ""))
+    (should (equal (bindat-unpack spec "a\0") "a"))))
+
+(let ((spec (bindat-type :pack-var v
+                         (x strz :pack-val v)
+                         :unpack-val x)))
+  (ert-deftest bindat-test--strz-varlen-len ()
+    :expected-result :failed
+    (should (equal (bindat-length spec "") 1))
+    (should (equal (bindat-length spec "abc") 4)))
+
+  (ert-deftest bindat-test--strz-varlen-pack ()
+    :expected-result :failed
+    (should (equal (bindat-pack spec "") "\0"))
+    (should (equal (bindat-pack spec "abc") "\141\142\143\0")))
+
+  (ert-deftest bindat-test--strz-varlen-unpack ()
+    :expected-result :failed
+    (should (equal (bindat-unpack spec "\0") ""))
+    (should (equal (bindat-unpack spec "\141\142\143\0") "abc"))))
+
+(let ((spec '((x strz 2))))
+  (ert-deftest bindat-test--strz-legacy-fixedlen-len ()
+    (should (equal (bindat-length spec '((x . ""))) 2))
+    (should (equal (bindat-length spec '((x . "a"))) 2)))
+
+  (ert-deftest bindat-test--strz-legacy-fixedlen-len-overflow ()
+    (should (equal (bindat-length spec '((x . "abc"))) 2)))
+
+  (ert-deftest bindat-test--strz-legacy-fixedlen-pack ()
+    (should (equal (bindat-pack spec '((x . ""))) "\0\0"))
+    (should (equal (bindat-pack spec '((x . "a"))) "\141\0")))
+
+  (ert-deftest bindat-test--strz-legacy-fixedlen-pack-overflow ()
+    :expected-result :failed
+    (should (equal (bindat-pack spec '((x . "abc"))) "\141\0")))
+
+  (ert-deftest bindat-test--strz-legacy-fixedlen-unpack ()
+    (should (equal (bindat-unpack spec "\0\0") '((x . ""))))
+    (should (equal (bindat-unpack spec "a\0") '((x . "a"))))))
+
 ;;; bindat-tests.el ends here
-- 
2.36.1


[-- Attachment #1.1.3: 0002-bindat-strz-Fix-off-by-one-bug-in-computed-length.patch --]
[-- Type: text/x-patch, Size: 1972 bytes --]

From af6899c1f6ecdeb75fcb43bea603e53cbe7a5f04 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sat, 28 May 2022 23:10:44 -0400
Subject: [PATCH 2/7] bindat (strz): Fix off-by-one bug in computed length

* lisp/emacs-lisp/bindat.el (strz): Include null terminator when
computing packed string length.
* test/lisp/emacs-lisp/bindat-tests.el (strz): Mark tests as passing.
---
 lisp/emacs-lisp/bindat.el            | 7 +++----
 test/lisp/emacs-lisp/bindat-tests.el | 2 --
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index c6d64975ec..f66458296a 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -687,10 +687,9 @@ bindat--type
   (bindat--pcase op
     ('unpack `(bindat--unpack-strz ,len))
     (`(length ,val)
-     `(cl-incf bindat-idx ,(cond
-                            ((null len) `(length ,val))
-                            ((numberp len) len)
-                            (t `(or ,len (length ,val))))))
+     `(cl-incf bindat-idx ,(if (numberp len)
+                               len
+                             `(1+ (length ,val)))))
     (`(pack . ,args)
      (macroexp-let2 nil len len
        `(if ,len
diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
index 53c0c359d8..ea6d110b8b 100644
--- a/test/lisp/emacs-lisp/bindat-tests.el
+++ b/test/lisp/emacs-lisp/bindat-tests.el
@@ -188,12 +188,10 @@ bindat-test--recursive
                          (x strz :pack-val v)
                          :unpack-val x)))
   (ert-deftest bindat-test--strz-varlen-len ()
-    :expected-result :failed
     (should (equal (bindat-length spec "") 1))
     (should (equal (bindat-length spec "abc") 4)))
 
   (ert-deftest bindat-test--strz-varlen-pack ()
-    :expected-result :failed
     (should (equal (bindat-pack spec "") "\0"))
     (should (equal (bindat-pack spec "abc") "\141\142\143\0")))
 
-- 
2.36.1


[-- Attachment #1.1.4: 0003-bindat-strz-Consistent-length-type-check.patch --]
[-- Type: text/x-patch, Size: 1594 bytes --]

From e504d1e850a544d6d139a51beee7ad5c2155c079 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sat, 28 May 2022 23:53:51 -0400
Subject: [PATCH 3/7] ; bindat (strz): Consistent length type check

The strz length computation uses `numberp' to switch between
fixed-length and variable-length modes, so packing should too.
---
 lisp/emacs-lisp/bindat.el | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index f66458296a..d64be721b2 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -692,13 +692,13 @@ bindat--type
                              `(1+ (length ,val)))))
     (`(pack . ,args)
      (macroexp-let2 nil len len
-       `(if ,len
-            ;; Same as non-zero terminated strings 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 . ,args)
-          (bindat--pack-strz . ,args))))))
+       (if (numberp len)
+           ;; Same as non-zero terminated strings 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 . ,args)
+         `(bindat--pack-strz . ,args))))))
 
 (cl-defmethod bindat--type (op (_ (eql 'bits))  len)
   (bindat--pcase op
-- 
2.36.1


[-- Attachment #1.1.5: 0004-bindat-strz-Always-write-null-terminator.patch --]
[-- Type: text/x-patch, Size: 2270 bytes --]

From a7613f605ae0acbe3a3fd8cde397464706c88952 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 29 May 2022 17:31:18 -0400
Subject: [PATCH 4/7] bindat (strz): Always write null terminator

* lisp/emacs-lisp/bindat.el (strz): When specifying a fixed length for
a packed strz string, make sure the null terminator is always written
even if the length of the string to pack is greater than or equal to
the fixed length.
* test/lisp/emacs-lisp/bindat-tests.el (strz): Mark test as passing.
---
 lisp/emacs-lisp/bindat.el            | 12 +++++++-----
 test/lisp/emacs-lisp/bindat-tests.el |  1 -
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index d64be721b2..12b2d20981 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -693,11 +693,13 @@ bindat--type
     (`(pack . ,args)
      (macroexp-let2 nil len len
        (if (numberp len)
-           ;; Same as non-zero terminated strings 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 . ,args)
+           `(progn
+              ;; Same as the str type, except always leave room for the null
+              ;; terminator. This assumes that `len' > 0.
+              (bindat--pack-str ,(1- len) . ,args)
+              ;; "Write" the null terminator. This assumes that `bindat-raw' was
+              ;; initialized with zeroes.
+              (setq bindat-idx (1+ bindat-idx)))
          `(bindat--pack-strz . ,args))))))
 
 (cl-defmethod bindat--type (op (_ (eql 'bits))  len)
diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
index ea6d110b8b..5152f67c01 100644
--- a/test/lisp/emacs-lisp/bindat-tests.el
+++ b/test/lisp/emacs-lisp/bindat-tests.el
@@ -177,7 +177,6 @@ bindat-test--recursive
     (should (equal (bindat-pack spec "a") "\141\0")))
 
   (ert-deftest bindat-test--strz-fixedlen-pack-overflow ()
-    :expected-result :failed
     (should (equal (bindat-pack spec "abc") "\141\0")))
 
   (ert-deftest bindat-test--strz-fixedlen-unpack ()
-- 
2.36.1


[-- Attachment #1.1.6: 0005-bindat-strz-Fix-wrong-type-argument-error-when-unpac.patch --]
[-- Type: text/x-patch, Size: 1828 bytes --]

From 787ea356aba16bcf9dd1f41d55e3ab7e1129215c Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 29 May 2022 18:09:08 -0400
Subject: [PATCH 5/7] bindat (strz): Fix wrong-type-argument error when
 unpacking

* lisp/emacs-lisp/bindat.el (strz): Fix (wrong-type-argument
number-or-marker-p nil) error when unpacking a strz with
unspecified (variable) length.
* test/lisp/emacs-lisp/bindat-tests.el (strz): Mark test as passing.
---
 lisp/emacs-lisp/bindat.el            | 4 ++--
 test/lisp/emacs-lisp/bindat-tests.el | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index 12b2d20981..20095ef6cd 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -165,12 +165,12 @@ bindat--unpack-str
     (if (stringp s) s
       (apply #'unibyte-string s))))
 
-(defun bindat--unpack-strz (len)
+(defun bindat--unpack-strz (&optional len)
   (let ((i 0) s)
     (while (and (if len (< i len) t) (/= (aref bindat-raw (+ bindat-idx i)) 0))
       (setq i (1+ i)))
     (setq s (substring bindat-raw bindat-idx (+ bindat-idx i)))
-    (setq bindat-idx (+ bindat-idx len))
+    (setq bindat-idx (+ bindat-idx (or len (1+ i))))
     (if (stringp s) s
       (apply #'unibyte-string s))))
 
diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
index 5152f67c01..01f9d51ad0 100644
--- a/test/lisp/emacs-lisp/bindat-tests.el
+++ b/test/lisp/emacs-lisp/bindat-tests.el
@@ -195,7 +195,6 @@ bindat-test--recursive
     (should (equal (bindat-pack spec "abc") "\141\142\143\0")))
 
   (ert-deftest bindat-test--strz-varlen-unpack ()
-    :expected-result :failed
     (should (equal (bindat-unpack spec "\0") ""))
     (should (equal (bindat-unpack spec "\141\142\143\0") "abc"))))
 
-- 
2.36.1


[-- Attachment #1.1.7: 0006-bindat-strz-Move-all-pack-logic-to-pack-function.patch --]
[-- Type: text/x-patch, Size: 2273 bytes --]

From 830eeae41522ce5885021c189e0d03c071fe4fc6 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 29 May 2022 21:23:57 -0400
Subject: [PATCH 6/7] ; bindat (strz): Move all pack logic to pack function

---
 lisp/emacs-lisp/bindat.el | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index 20095ef6cd..cf17e5764d 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -439,11 +439,19 @@ bindat--pack-str
     (aset bindat-raw (+ bindat-idx i) (aref v i)))
   (setq bindat-idx (+ bindat-idx len)))
 
-(defun bindat--pack-strz (v)
-  (let ((len (length v)))
-    (dotimes (i len)
-      (aset bindat-raw (+ bindat-idx i) (aref v i)))
-    (setq bindat-idx (+ bindat-idx len 1))))
+(defun bindat--pack-strz (len v)
+  (if (numberp len)
+      (progn
+        ;; Same as the str type, except always leave room for the null
+        ;; terminator. This assumes that `len' > 0.
+        (bindat--pack-str (1- len) v)
+        ;; "Write" the null terminator. This assumes that `bindat-raw' was
+        ;; initialized with zeroes.
+        (setq bindat-idx (1+ bindat-idx)))
+    (let ((len (length v)))
+      (dotimes (i len)
+        (aset bindat-raw (+ bindat-idx i) (aref v i)))
+      (setq bindat-idx (+ bindat-idx len 1)))))
 
 (defun bindat--pack-bits (len v)
   (let ((bnum (1- (* 8 len))) j m)
@@ -690,17 +698,7 @@ bindat--type
      `(cl-incf bindat-idx ,(if (numberp len)
                                len
                              `(1+ (length ,val)))))
-    (`(pack . ,args)
-     (macroexp-let2 nil len len
-       (if (numberp len)
-           `(progn
-              ;; Same as the str type, except always leave room for the null
-              ;; terminator. This assumes that `len' > 0.
-              (bindat--pack-str ,(1- len) . ,args)
-              ;; "Write" the null terminator. This assumes that `bindat-raw' was
-              ;; initialized with zeroes.
-              (setq bindat-idx (1+ bindat-idx)))
-         `(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


[-- Attachment #1.1.8: 0007-bindat-strz-Fix-packing-of-long-strings-with-legacy-.patch --]
[-- Type: text/x-patch, Size: 1700 bytes --]

From 08d1666b99533f8a72a4ef2c8abe77be8058d0e0 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 29 May 2022 21:46:16 -0400
Subject: [PATCH 7/7] bindat (strz): Fix packing of long strings with
 legacy-style spec

* lisp/emacs-lisp/bindat.el (strz): Call the proper handler when
packing to fix lack of null terminator when packing strings with
length greater than or equal to the declared length.
* test/lisp/emacs-lisp/bindat-tests.el (strz): Mark test as passing.
---
 lisp/emacs-lisp/bindat.el            | 3 ++-
 test/lisp/emacs-lisp/bindat-tests.el | 1 -
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index cf17e5764d..e8eb59e24d 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -480,7 +480,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)
diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
index 01f9d51ad0..ca93a2468b 100644
--- a/test/lisp/emacs-lisp/bindat-tests.el
+++ b/test/lisp/emacs-lisp/bindat-tests.el
@@ -211,7 +211,6 @@ bindat-test--recursive
     (should (equal (bindat-pack spec '((x . "a"))) "\141\0")))
 
   (ert-deftest bindat-test--strz-legacy-fixedlen-pack-overflow ()
-    :expected-result :failed
     (should (equal (bindat-pack spec '((x . "abc"))) "\141\0")))
 
   (ert-deftest bindat-test--strz-legacy-fixedlen-unpack ()
-- 
2.36.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: bug#55719: [PATCH] bindat strz fixes

[Eli Zaretskii]

> Cc: 55719@debbugs.gnu.org
> Date: Mon, 30 May 2022 12:53:31 -0400
> From: Richard Hansen <rhansen@rhansen.org>
> 
> The attached series of patches should fix bug #55719 (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=55719).

Aren't those all due to the issue of including the terminating null
byte in a packed string?  And if so, I wonder whether indeed the null
byte should be included, since that means you cannot handle strings
that include null bytes as part of the payload, not as terminators.

Can you tell why you are convinced the null byte should be considered
as part of the string?

Thanks.

bug#55719: [PATCH] bindat strz fixes

[Richard Hansen]

[-- Attachment #1.1: Type: text/plain, Size: 621 bytes --]

On 5/31/22 07:08, Eli Zaretskii wrote:
> Can you tell why you are convinced the null byte should be considered
> as part of the string?

The null terminator is the reason one would use the strz type. If the
user doesn't want a null terminator, they should use the str type
instead. From the documentation [1]:

> str len
>
>     String of bytes of length len.
>
> strz &optional len
>
>     Zero-terminated string of bytes, can be of arbitrary length or 
>     in a fixed-size field with length len.

[1] https://www.gnu.org/software/emacs/manual/html_node/elisp/Bindat-Types.html#index-bindat_002dtype

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

bug#55719: [PATCH] bindat strz fixes

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
> index 7722cf6c02..53c0c359d8 100644
> --- a/test/lisp/emacs-lisp/bindat-tests.el
> +++ b/test/lisp/emacs-lisp/bindat-tests.el
> @@ -162,4 +162,64 @@ bindat-test--recursive
>                                          (bindat-pack bindat-test--LEB128 n))
>                           n)))))))
>  
> +(let ((spec (bindat-type :pack-var v
> +                         (x strz 2 :pack-val v)
> +                         :unpack-val x)))

Any particular reason why you define it this way instead of just

    (bindat-type strz 2)

?

> +  (ert-deftest bindat-test--strz-fixedlen-len ()
> +    (should (equal (bindat-length spec "") 2))
> +    (should (equal (bindat-length spec "a") 2)))
> +
> +  (ert-deftest bindat-test--strz-fixedlen-len-overflow ()
> +    (should (equal (bindat-length spec "abc") 2)))
> +
> +  (ert-deftest bindat-test--strz-fixedlen-pack ()
> +    (should (equal (bindat-pack spec "") "\0\0"))
> +    (should (equal (bindat-pack spec "a") "\141\0")))

LGTM.

> +  (ert-deftest bindat-test--strz-fixedlen-pack-overflow ()
> +    :expected-result :failed
> +    (should (equal (bindat-pack spec "abc") "\141\0")))

I think this changes the intended semantics.  Until now `strz N` has
meant that N bytes are used to encode the string and that it can
hold upto a string of length N (in which case there's no terminating NUL
byte).  I agree that it's not the only valid semantics, but I'm not sure
we want to change it at this point.

Do you have a particular reason to make this change.

> +  (ert-deftest bindat-test--strz-fixedlen-unpack ()
> +    (should (equal (bindat-unpack spec "\0\0") ""))
> +    (should (equal (bindat-unpack spec "a\0") "a"))))

How 'bout

     (bindat-unpack spec "ab")

?

> +(let ((spec (bindat-type :pack-var v
> +                         (x strz :pack-val v)
> +                         :unpack-val x)))

Similarly here, I'd use just (bindat-type strz)

> +  (ert-deftest bindat-test--strz-varlen-len ()
> +    :expected-result :failed
> +    (should (equal (bindat-length spec "") 1))
> +    (should (equal (bindat-length spec "abc") 4)))
> +
> +  (ert-deftest bindat-test--strz-varlen-pack ()
> +    :expected-result :failed
> +    (should (equal (bindat-pack spec "") "\0"))
> +    (should (equal (bindat-pack spec "abc") "\141\142\143\0")))
> +
> +  (ert-deftest bindat-test--strz-varlen-unpack ()
> +    :expected-result :failed
> +    (should (equal (bindat-unpack spec "\0") ""))
> +    (should (equal (bindat-unpack spec "\141\142\143\0") "abc"))))

Looks good (tho I'd write "abc\0" i.s.o "\141\142\143\0").
Not sure what we should do about (bindat-unpack spec "abc")?

> diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
> index c6d64975ec..f66458296a 100644
> --- a/lisp/emacs-lisp/bindat.el
> +++ b/lisp/emacs-lisp/bindat.el
> @@ -687,10 +687,9 @@ bindat--type
>    (bindat--pcase op
>      ('unpack `(bindat--unpack-strz ,len))
>      (`(length ,val)
> -     `(cl-incf bindat-idx ,(cond
> -                            ((null len) `(length ,val))
> -                            ((numberp len) len)
> -                            (t `(or ,len (length ,val))))))
> +     `(cl-incf bindat-idx ,(if (numberp len)
> +                               len
> +                             `(1+ (length ,val)))))

`len` is supposed to be an ELisp *expression*.  E.g. it can be

    (+ a 4)

in which case (numberp len) will fail yet we should return the value of
`len` rather than (1+ (length ,val)).  In the original code, the cases
for (null len) and (numberp len) are *optimizations*.

I haven't yet looked at the rest of the patches.  If you can update your
patches based on this feedback, that would be great, but in the worst
case, I'll get to reviewing the rest sooner or later anyway.


        Stefan

bug#55719: [PATCH] bindat strz fixes

[Richard Hansen]

[-- Attachment #1.1.1: Type: text/plain, Size: 2832 bytes --]

Thank you for the review! I have attached a new set of patches.

On 5/31/22 19:00, Stefan Monnier wrote:
>> +(let ((spec (bindat-type :pack-var v
>> +                         (x strz 2 :pack-val v)
>> +                         :unpack-val x)))
> 
> Any particular reason why you define it this way instead of just
> 
>      (bindat-type strz 2)
> 
> ?

Thanks, I should have realized that was possible. Fixed.

>> +  (ert-deftest bindat-test--strz-fixedlen-pack-overflow ()
>> +    :expected-result :failed
>> +    (should (equal (bindat-pack spec "abc") "\141\0")))
> 
> I think this changes the intended semantics.  Until now `strz N` has
> meant that N bytes are used to encode the string and that it can
> hold upto a string of length N (in which case there's no terminating NUL
> byte).  I agree that it's not the only valid semantics, but I'm not sure
> we want to change it at this point.
> 
> Do you have a particular reason to make this change.

A few:
   * The documentation says that the packed output is null terminated
     so that's what users expect.
   * It is safer (packed output is less likely to cause some other
     program to run past the end of the field).
   * Without this change, there is no difference between `strz N` and
     `str N`. So what would be the point of `strz N`?
   * If the user selected strz, the application probably requires null
     termination in all cases, not just when the string is under a
     certain length.

>> +  (ert-deftest bindat-test--strz-fixedlen-unpack ()
>> +    (should (equal (bindat-unpack spec "\0\0") ""))
>> +    (should (equal (bindat-unpack spec "a\0") "a"))))
> 
> How 'bout
> 
>       (bindat-unpack spec "ab")
> 
> ?

I added some comments explaining why cases like that aren't tested.

> (tho I'd write "abc\0" i.s.o "\141\142\143\0").

Done.

> Not sure what we should do about (bindat-unpack spec "abc")?

That is not a valid packed string, so I'm not too concerned about it.
Currently it signals an args-out-of-range error because it tries to
read past the end of the string, which seems like an acceptable
behavior to me. We could make the error message more friendly, such as
"end of input reached while searching for null terminator", but I'd
rather not make that change right now with this set of patches.

>> -     `(cl-incf bindat-idx ,(cond
>> -                            ((null len) `(length ,val))
>> -                            ((numberp len) len)
>> -                            (t `(or ,len (length ,val))))))
>> +     `(cl-incf bindat-idx ,(if (numberp len)
>> +                               len
>> +                             `(1+ (length ,val)))))
> 
> `len` is supposed to be an ELisp *expression*.

Ah, I was wondering why it was written that way. Fixed.

[-- Attachment #1.1.2: 0001-bindat-tests-strz-Add-more-tests.patch --]
[-- Type: text/x-patch, Size: 3535 bytes --]

From e07c58f24608806d81417f70df4d904b4c5fffe3 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 29 May 2022 17:15:04 -0400
Subject: [PATCH 1/7] ; bindat-tests (strz): Add more tests

---
 test/lisp/emacs-lisp/bindat-tests.el | 65 ++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
index 7722cf6c02..fbe574e490 100644
--- a/test/lisp/emacs-lisp/bindat-tests.el
+++ b/test/lisp/emacs-lisp/bindat-tests.el
@@ -162,4 +162,69 @@ bindat-test--recursive
                                         (bindat-pack bindat-test--LEB128 n))
                          n)))))))
 
+(let ((spec (bindat-type strz 2)))
+  (ert-deftest bindat-test--strz-fixedlen-len ()
+    (should (equal (bindat-length spec "") 2))
+    (should (equal (bindat-length spec "a") 2)))
+
+  (ert-deftest bindat-test--strz-fixedlen-len-overflow ()
+    (should (equal (bindat-length spec "abc") 2)))
+
+  (ert-deftest bindat-test--strz-fixedlen-pack ()
+    (should (equal (bindat-pack spec "") "\0\0"))
+    (should (equal (bindat-pack spec "a") "a\0")))
+
+  (ert-deftest bindat-test--strz-fixedlen-pack-overflow ()
+    :expected-result :failed
+    (should (equal (bindat-pack spec "abc") "a\0")))
+
+  (ert-deftest bindat-test--strz-fixedlen-unpack ()
+    ;; There are no tests for unpacking "ab" or "ab\0" because those
+    ;; packed strings cannot be produced from the spec (packing "ab"
+    ;; should produce "a\0", not "ab" or "ab\0").
+    (should (equal (bindat-unpack spec "\0\0") ""))
+    (should (equal (bindat-unpack spec "a\0") "a"))))
+
+(let ((spec (bindat-type strz)))
+  (ert-deftest bindat-test--strz-varlen-len ()
+    :expected-result :failed
+    (should (equal (bindat-length spec "") 1))
+    (should (equal (bindat-length spec "abc") 4)))
+
+  (ert-deftest bindat-test--strz-varlen-pack ()
+    :expected-result :failed
+    (should (equal (bindat-pack spec "") "\0"))
+    (should (equal (bindat-pack spec "abc") "abc\0")))
+
+  (ert-deftest bindat-test--strz-varlen-unpack ()
+    :expected-result :failed
+    ;; There is no test for unpacking a string without a null
+    ;; terminator because such packed strings cannot be produced from
+    ;; the spec (packing "a" should produce "a\0", not "a").
+    (should (equal (bindat-unpack spec "\0") ""))
+    (should (equal (bindat-unpack spec "abc\0") "abc"))))
+
+(let ((spec '((x strz 2))))
+  (ert-deftest bindat-test--strz-legacy-fixedlen-len ()
+    (should (equal (bindat-length spec '((x . ""))) 2))
+    (should (equal (bindat-length spec '((x . "a"))) 2)))
+
+  (ert-deftest bindat-test--strz-legacy-fixedlen-len-overflow ()
+    (should (equal (bindat-length spec '((x . "abc"))) 2)))
+
+  (ert-deftest bindat-test--strz-legacy-fixedlen-pack ()
+    (should (equal (bindat-pack spec '((x . ""))) "\0\0"))
+    (should (equal (bindat-pack spec '((x . "a"))) "a\0")))
+
+  (ert-deftest bindat-test--strz-legacy-fixedlen-pack-overflow ()
+    :expected-result :failed
+    (should (equal (bindat-pack spec '((x . "abc"))) "a\0")))
+
+  (ert-deftest bindat-test--strz-legacy-fixedlen-unpack ()
+    ;; There are no tests for unpacking "ab" or "ab\0" because those
+    ;; packed strings cannot be produced from the spec (packing "ab"
+    ;; should produce "a\0", not "ab" or "ab\0").
+    (should (equal (bindat-unpack spec "\0\0") '((x . ""))))
+    (should (equal (bindat-unpack spec "a\0") '((x . "a"))))))
+
 ;;; bindat-tests.el ends here
-- 
2.36.1


[-- Attachment #1.1.3: 0002-bindat-strz-Fix-off-by-one-bug-in-computed-length.patch --]
[-- Type: text/x-patch, Size: 1877 bytes --]

From fa1835bd83fc8e64426023fe979079bfa21cbdcb Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sat, 28 May 2022 23:10:44 -0400
Subject: [PATCH 2/7] bindat (strz): Fix off-by-one bug in computed length

* lisp/emacs-lisp/bindat.el (strz): Include null terminator when
computing packed string length.
* test/lisp/emacs-lisp/bindat-tests.el (strz): Mark tests as passing.
---
 lisp/emacs-lisp/bindat.el            | 4 ++--
 test/lisp/emacs-lisp/bindat-tests.el | 2 --
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index c6d64975ec..b236e47e5b 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -688,9 +688,9 @@ bindat--type
     ('unpack `(bindat--unpack-strz ,len))
     (`(length ,val)
      `(cl-incf bindat-idx ,(cond
-                            ((null len) `(length ,val))
+                            ((null len) `(1+ (length ,val)))
                             ((numberp len) len)
-                            (t `(or ,len (length ,val))))))
+                            (t `(or ,len (1+ (length ,val)))))))
     (`(pack . ,args)
      (macroexp-let2 nil len len
        `(if ,len
diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
index fbe574e490..6added9390 100644
--- a/test/lisp/emacs-lisp/bindat-tests.el
+++ b/test/lisp/emacs-lisp/bindat-tests.el
@@ -187,12 +187,10 @@ bindat-test--recursive
 
 (let ((spec (bindat-type strz)))
   (ert-deftest bindat-test--strz-varlen-len ()
-    :expected-result :failed
     (should (equal (bindat-length spec "") 1))
     (should (equal (bindat-length spec "abc") 4)))
 
   (ert-deftest bindat-test--strz-varlen-pack ()
-    :expected-result :failed
     (should (equal (bindat-pack spec "") "\0"))
     (should (equal (bindat-pack spec "abc") "abc\0")))
 
-- 
2.36.1


[-- Attachment #1.1.4: 0003-bindat-strz-Consistent-length-type-check.patch --]
[-- Type: text/x-patch, Size: 1607 bytes --]

From 34dc1a4d39f9c11628b0c9a2bb3c080d6df20983 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sat, 28 May 2022 23:53:51 -0400
Subject: [PATCH 3/7] ; bindat (strz): Consistent length type check

The strz length computation uses `numberp' to switch between
fixed-length and variable-length modes, so packing should too.
---
 lisp/emacs-lisp/bindat.el | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index b236e47e5b..1a7ff625ee 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -693,13 +693,13 @@ bindat--type
                             (t `(or ,len (1+ (length ,val)))))))
     (`(pack . ,args)
      (macroexp-let2 nil len len
-       `(if ,len
-            ;; Same as non-zero terminated strings 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 . ,args)
-          (bindat--pack-strz . ,args))))))
+       (if (numberp len)
+           ;; Same as non-zero terminated strings 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 . ,args)
+         `(bindat--pack-strz . ,args))))))
 
 (cl-defmethod bindat--type (op (_ (eql 'bits))  len)
   (bindat--pcase op
-- 
2.36.1


[-- Attachment #1.1.5: 0004-bindat-strz-Always-write-null-terminator.patch --]
[-- Type: text/x-patch, Size: 2264 bytes --]

From 62af0a06d0e0cc904e8b33b1eed675f68e47892c Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 29 May 2022 17:31:18 -0400
Subject: [PATCH 4/7] bindat (strz): Always write null terminator

* lisp/emacs-lisp/bindat.el (strz): When specifying a fixed length for
a packed strz string, make sure the null terminator is always written
even if the length of the string to pack is greater than or equal to
the fixed length.
* test/lisp/emacs-lisp/bindat-tests.el (strz): Mark test as passing.
---
 lisp/emacs-lisp/bindat.el            | 12 +++++++-----
 test/lisp/emacs-lisp/bindat-tests.el |  1 -
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index 1a7ff625ee..68d30e088c 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -694,11 +694,13 @@ bindat--type
     (`(pack . ,args)
      (macroexp-let2 nil len len
        (if (numberp len)
-           ;; Same as non-zero terminated strings 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 . ,args)
+           `(progn
+              ;; Same as the str type, except always leave room for the null
+              ;; terminator. This assumes that `len' > 0.
+              (bindat--pack-str ,(1- len) . ,args)
+              ;; "Write" the null terminator. This assumes that `bindat-raw' was
+              ;; initialized with zeroes.
+              (setq bindat-idx (1+ bindat-idx)))
          `(bindat--pack-strz . ,args))))))
 
 (cl-defmethod bindat--type (op (_ (eql 'bits))  len)
diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
index 6added9390..baf4313170 100644
--- a/test/lisp/emacs-lisp/bindat-tests.el
+++ b/test/lisp/emacs-lisp/bindat-tests.el
@@ -175,7 +175,6 @@ bindat-test--recursive
     (should (equal (bindat-pack spec "a") "a\0")))
 
   (ert-deftest bindat-test--strz-fixedlen-pack-overflow ()
-    :expected-result :failed
     (should (equal (bindat-pack spec "abc") "a\0")))
 
   (ert-deftest bindat-test--strz-fixedlen-unpack ()
-- 
2.36.1


[-- Attachment #1.1.6: 0005-bindat-strz-Fix-wrong-type-argument-error-when-unpac.patch --]
[-- Type: text/x-patch, Size: 1894 bytes --]

From ca088f5fc116542c7e19136eb3546b71200af612 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 29 May 2022 18:09:08 -0400
Subject: [PATCH 5/7] bindat (strz): Fix wrong-type-argument error when
 unpacking

* lisp/emacs-lisp/bindat.el (strz): Fix (wrong-type-argument
number-or-marker-p nil) error when unpacking a strz with
unspecified (variable) length.
* test/lisp/emacs-lisp/bindat-tests.el (strz): Mark test as passing.
---
 lisp/emacs-lisp/bindat.el            | 4 ++--
 test/lisp/emacs-lisp/bindat-tests.el | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index 68d30e088c..99b896998f 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -165,12 +165,12 @@ bindat--unpack-str
     (if (stringp s) s
       (apply #'unibyte-string s))))
 
-(defun bindat--unpack-strz (len)
+(defun bindat--unpack-strz (&optional len)
   (let ((i 0) s)
     (while (and (if len (< i len) t) (/= (aref bindat-raw (+ bindat-idx i)) 0))
       (setq i (1+ i)))
     (setq s (substring bindat-raw bindat-idx (+ bindat-idx i)))
-    (setq bindat-idx (+ bindat-idx len))
+    (setq bindat-idx (+ bindat-idx (or len (1+ i))))
     (if (stringp s) s
       (apply #'unibyte-string s))))
 
diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
index baf4313170..58c8144ac6 100644
--- a/test/lisp/emacs-lisp/bindat-tests.el
+++ b/test/lisp/emacs-lisp/bindat-tests.el
@@ -194,7 +194,6 @@ bindat-test--recursive
     (should (equal (bindat-pack spec "abc") "abc\0")))
 
   (ert-deftest bindat-test--strz-varlen-unpack ()
-    :expected-result :failed
     ;; There is no test for unpacking a string without a null
     ;; terminator because such packed strings cannot be produced from
     ;; the spec (packing "a" should produce "a\0", not "a").
-- 
2.36.1


[-- Attachment #1.1.7: 0006-bindat-strz-Move-all-pack-logic-to-pack-function.patch --]
[-- Type: text/x-patch, Size: 2315 bytes --]

From 55c5555fae47b52b3881573f005db49f27991236 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 29 May 2022 21:23:57 -0400
Subject: [PATCH 6/7] ; bindat (strz): Move all pack logic to pack function

---
 lisp/emacs-lisp/bindat.el | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index 99b896998f..b02d19ffba 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -439,11 +439,19 @@ bindat--pack-str
     (aset bindat-raw (+ bindat-idx i) (aref v i)))
   (setq bindat-idx (+ bindat-idx len)))
 
-(defun bindat--pack-strz (v)
-  (let ((len (length v)))
-    (dotimes (i len)
-      (aset bindat-raw (+ bindat-idx i) (aref v i)))
-    (setq bindat-idx (+ bindat-idx len 1))))
+(defun bindat--pack-strz (len v)
+  (if (numberp len)
+      (progn
+        ;; Same as the str type, except always leave room for the null
+        ;; terminator. This assumes that `len' > 0.
+        (bindat--pack-str (1- len) v)
+        ;; "Write" the null terminator. This assumes that `bindat-raw' was
+        ;; initialized with zeroes.
+        (setq bindat-idx (1+ bindat-idx)))
+    (let ((len (length v)))
+      (dotimes (i len)
+        (aset bindat-raw (+ bindat-idx i) (aref v i)))
+      (setq bindat-idx (+ bindat-idx len 1)))))
 
 (defun bindat--pack-bits (len v)
   (let ((bnum (1- (* 8 len))) j m)
@@ -691,17 +699,7 @@ bindat--type
                             ((null len) `(1+ (length ,val)))
                             ((numberp len) len)
                             (t `(or ,len (1+ (length ,val)))))))
-    (`(pack . ,args)
-     (macroexp-let2 nil len len
-       (if (numberp len)
-           `(progn
-              ;; Same as the str type, except always leave room for the null
-              ;; terminator. This assumes that `len' > 0.
-              (bindat--pack-str ,(1- len) . ,args)
-              ;; "Write" the null terminator. This assumes that `bindat-raw' was
-              ;; initialized with zeroes.
-              (setq bindat-idx (1+ bindat-idx)))
-         `(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


[-- Attachment #1.1.8: 0007-bindat-strz-Fix-packing-of-long-strings-with-legacy-.patch --]
[-- Type: text/x-patch, Size: 1694 bytes --]

From b794ae3957bcd7efe54d551a244856c1503d4b03 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Sun, 29 May 2022 21:46:16 -0400
Subject: [PATCH 7/7] bindat (strz): Fix packing of long strings with
 legacy-style spec

* lisp/emacs-lisp/bindat.el (strz): Call the proper handler when
packing to fix lack of null terminator when packing strings with
length greater than or equal to the declared length.
* test/lisp/emacs-lisp/bindat-tests.el (strz): Mark test as passing.
---
 lisp/emacs-lisp/bindat.el            | 3 ++-
 test/lisp/emacs-lisp/bindat-tests.el | 1 -
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index b02d19ffba..9ed1ab1065 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -480,7 +480,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)
diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
index 58c8144ac6..df03463475 100644
--- a/test/lisp/emacs-lisp/bindat-tests.el
+++ b/test/lisp/emacs-lisp/bindat-tests.el
@@ -213,7 +213,6 @@ bindat-test--recursive
     (should (equal (bindat-pack spec '((x . "a"))) "a\0")))
 
   (ert-deftest bindat-test--strz-legacy-fixedlen-pack-overflow ()
-    :expected-result :failed
     (should (equal (bindat-pack spec '((x . "abc"))) "a\0")))
 
   (ert-deftest bindat-test--strz-legacy-fixedlen-unpack ()
-- 
2.36.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

bug#55719: [PATCH] bindat strz fixes

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> +  (ert-deftest bindat-test--strz-fixedlen-pack-overflow ()
>>> +    :expected-result :failed
>>> +    (should (equal (bindat-pack spec "abc") "\141\0")))
>> I think this changes the intended semantics.  Until now `strz N` has
>> meant that N bytes are used to encode the string and that it can
>> hold upto a string of length N (in which case there's no terminating NUL
>> byte).  I agree that it's not the only valid semantics, but I'm not sure
>> we want to change it at this point.
>> Do you have a particular reason to make this change.
>
> A few:
>   * The documentation says that the packed output is null terminated
>     so that's what users expect.
>   * It is safer (packed output is less likely to cause some other
>     program to run past the end of the field).
>   * Without this change, there is no difference between `strz N` and
>     `str N`. So what would be the point of `strz N`?
>   * If the user selected strz, the application probably requires null
>     termination in all cases, not just when the string is under a
>     certain length.

You're listing advantages of this choice, which we know about already.
The other choice also has advantages.  These don't count as "particular
reasons" (e.g. a real-life concrete *need* for it, rather than a mere
preference).

The particular reason to prefer the current behavior is
backward compatibility (which we could call "inertia").

Note also that `strz` without a length (or with a nil length) behaves
the way you want.

Of course, we could add an additional (optional) arg to `strz` to
specify what should happen when unpacking a string with missing NUL byte
as well as whether to truncate to N-1 chars rather than to N chars to
make sure there is a terminating NUL byte.

>>> +  (ert-deftest bindat-test--strz-fixedlen-unpack ()
>>> +    (should (equal (bindat-unpack spec "\0\0") ""))
>>> +    (should (equal (bindat-unpack spec "a\0") "a"))))
>> How 'bout
>>       (bindat-unpack spec "ab")
>> ?
>
> I added some comments explaining why cases like that aren't tested.

The byte-string to unpack is not necessarily built from our own code
(usually bindat is used to communicate with some other application), so
whether our code can generate "ab" is not really relevant: the question
still comes up about what we should do with "ab" (where valid answers
could be to return "ab" or to return "a" or to signal an error, ...).
Of course we can also decide it's "undefined".


        Stefan

bug#55719: [PATCH] bindat strz fixes

[Richard Hansen]

[-- Attachment #1.1: Type: text/plain, Size: 4431 bytes --]

On 6/1/22 08:04, Stefan Monnier wrote:
>>    * The documentation says that the packed output is null terminated
>>      so that's what users expect.
>>    * It is safer (packed output is less likely to cause some other
>>      program to run past the end of the field).
>>    * Without this change, there is no difference between `strz N` and
>>      `str N`. So what would be the point of `strz N`?
>>    * If the user selected strz, the application probably requires null
>>      termination in all cases, not just when the string is under a
>>      certain length.
> 
> You're listing advantages of this choice, which we know about already.
> The other choice also has advantages.  These don't count as "particular
> reasons" (e.g. a real-life concrete *need* for it, rather than a mere
> preference).

I don't have a concrete need for it at the moment. I just noticed the bug while I was fixing the other bugs. I can leave that change out of the patch series if that is the only thing impeding merge.

> The particular reason to prefer the current behavior is
> backward compatibility (which we could call "inertia").

Anyone that needs the current behavior should be using `str N`, not `strz N`. If they're using `strz N`, then I would consider that to be a bug in their code. If this change breaks someone, they can fix it easily: just change `strz N` to `str N`. I understand that we should endeavor to maintain compatibility, but keeping the current behavior would be intentionally preserving a bug to accommodate other bugs. I don't think that's a good trade-off.

> Note also that `strz` without a length (or with a nil length) behaves
> the way you want.

No -- strz without a length results in variable length encoding. Without these changes, the only way to get a fixed length output with guaranteed null termination is to do `(substring str 0 (1- N))` before packing. (Or explicitly pack a separate zero byte immediately after the `str N-1` or `strz N-1` entry.)

> Of course, we could add an additional (optional) arg to `strz` to
> specify what should happen when unpacking a string with missing NUL byte
> as well as whether to truncate to N-1 chars rather than to N chars to
> make sure there is a terminating NUL byte.

I don't think that's necessary. I think most use cases are satisfied by the current str behavior and the strz behavior with these patches.

>>> How 'bout
>>>        (bindat-unpack spec "ab")
>>> ?
>>
>> I added some comments explaining why cases like that aren't tested.
> 
> The byte-string to unpack is not necessarily built from our own code
> (usually bindat is used to communicate with some other application), so
> whether our code can generate "ab" is not really relevant: the question
> still comes up about what we should do with "ab" (where valid answers
> could be to return "ab" or to return "a" or to signal an error, ...).
> Of course we can also decide it's "undefined".

Regardless of what other applications produce, packed strings like that are invalid according to the spec provided to `bindat-unpack`. Attempting to do something useful with such invalid strings assumes that Postel's Law is a good thing, which I don't agree with (especially with security-sensitive protocols). I think it is safest to signal an error, which it currently does.

A real example of why it might be undesirable to attempt to process a strz string without a null terminator:

   1. The sender streams multiple packed messages over a reliable, in-order protocol like TCP or pipes.
   2. The sending system breaks up the messages at arbitrary places to fit in packets/buffers, so the receiving side might see an incomplete message that will be completed by a future packet (or multiple future packets).
   3. The receiver attempts to extract a message from the bytes received so far. If the bytes do not form a valid message according to the bindat-unpack spec, then assume the message was truncated. Remember the bytes received so far and wait for the next packet.
   4. When the next packet arrives, concatenate its bytes with the remembered bytes and try again.

If bindat-unpack did not signal an error when expecting a null terminator, then the above protocol would not work without extra effort by the user.

Either way, changing the way `bindat-unpack` handles invalid strings feels out of scope for this particular bug report.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

bug#55719: [PATCH] bindat strz fixes

[Richard Hansen]

On 6/1/22 16:23, Richard Hansen wrote:
> Anyone that needs the current behavior should be using `str N`, not 
> `strz N`. If they're using `strz N`, then I would consider that to be 
> a bug in their code. If this change breaks someone, they can fix it 
> easily: just change `strz N` to `str N`. I understand that we should 
> endeavor to maintain compatibility, but keeping the current behavior 
> would be intentionally preserving a bug to accommodate other bugs. I 
> don't think that's a good trade-off.

Actually I'm wrong; there is a difference between `str N` and `strz N`. With `str N`, all zero bytes in the input string are included in the unpacked value (including trailing null bytes). With `strz N`, the unpacked string ends just before the first zero byte.

I'll remove the `strz N` change from the patch series so that we can get the rest merged.

bug#55719: [PATCH] bindat strz fixes

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> I don't have a concrete need for it at the moment. I just noticed the bug
> while I was fixing the other bugs. I can leave that change out of the patch
> series if that is the only thing impeding merge.

I've installed the rest of the patch series into `master`, thanks.

>> The particular reason to prefer the current behavior is
>> backward compatibility (which we could call "inertia").
> Anyone that needs the current behavior should be using `str N`, not
> `strz N`.

`str N` does not give the same semantics when unpacking.

> If they're using `strz N`, then I would consider that to be a bug in
> their code.

It all depends how the format was defined.  "NUL-terminated N-bytes-max
strings" can be like the one currently supported or like the ones your
code supports.  Which one is better is irrelevant when the format is
not of your choosing.

I'm not at all opposed to supporting the kind of NUL-terminated strings
you propose but it should be *in addition* to the ones
already supported.

> If this change breaks someone, they can fix it easily: just
> change `strz N` to `str N`.

No, because unpacking "a\0" with (str 2) give "a\0" rather than "a".

> I understand that we should endeavor to maintain compatibility, but
> keeping the current behavior would be intentionally preserving a bug
> to accommodate other bugs.  I don't think that's a good trade-off.

It's only a bug if it doesn't match the format you need to use.
Your code introduces a bug when the format is defined to behave like the
current code :-(

>> Note also that `strz` without a length (or with a nil length) behaves
>> the way you want.
> No -- strz without a length results in variable length encoding. Without
> these changes, the only way to get a fixed length output with guaranteed
> null termination is to do `(substring str 0 (1- N))` before packing. (Or
> explicitly pack a separate zero byte immediately after the `str N-1` or
> `strz N-1` entry.)

...or define a new type.

>>>>        (bindat-unpack spec "ab")
[...]
> Regardless of what other applications produce, packed strings like that are
> invalid according to the spec provided to `bindat-unpack`.

Where in the spec/doc of Bindat do you see that "ab" is not a valid
input to `bindat-unpack` for `strz 2`?

> A real example of why it might be undesirable to attempt to process
> a strz string without a null terminator:

You don't need to convince me that the format you propose is useful.


        Stefan