bug#58669: 28.2; bindat: str and strz not operating on vectors

[Stefan Kangas <stefankangas@gmail.com>]

Nacho Barrientos <nacho.barrientos@cern.ch> writes:

> These two snippets:
>
>   (let* ((type (bindat-type
>                  (data strz)))
>          (data [#x4e #x6f #x6e #x65 #x00])
>          (unpacked (bindat-unpack type data)))
>     (bindat-get-field unpacked 'data))
>
>   (let* ((type (bindat-type
>                  (data str 4)))
>          (data [#x4e #x6f #x6e #x65])
>          (unpacked (bindat-unpack type data)))
>     (bindat-get-field unpacked 'data))
>
> Both fail similarly:

Copying in Stefan Monnier, in case he has any comments.

> Debugger entered--Lisp error: (wrong-type-argument listp [78 111 110 101])
>   apply(unibyte-string [78 111 110 101])
>   (if (stringp s) s (apply #'unibyte-string s))
>   (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 (or len (1+ i)))) (if (stringp s) s (apply #'unibyte-string s)))
>   bindat--unpack-strz(nil)
>   [...]
>
> Debugger entered--Lisp error: (wrong-type-argument listp [78 111 110 101])
>   apply(unibyte-string [78 111 110 101])
>   (if (stringp s) s (apply #'unibyte-string s))
>   (let ((s (substring bindat-raw bindat-idx (+ bindat-idx len)))) (setq bindat-idx (+ bindat-idx len)) (if (stringp s) s (apply #'unibyte-string s)))
>   bindat--unpack-str(4)
>   [...]
>
> With the attached patch both return the string "None" as expected (well,
> as I'd expect, *grins*). The diff also adds some extra tests.
>
> I'm running 28.2 but I've declared both functions as they're in the
> current master and the patch is on top of the current master, too.
>
> It's the first time that I look into this package so I'm not sure this
> is the way it's meant to work but similar operations work fine on
> vectors of bytes, for instance:
>
>   (let* ((type (bindat-type
>                  (first uint 16)
>                  (second uint 16)))
>          (data [#x00 #xff #xff #x00])
>          (unpacked (bindat-unpack type data)))
>     (list (bindat-get-field unpacked 'first)
>           (bindat-get-field unpacked 'second)
>           (bindat-length type unpacked)))
>
> (which returns (255 65280 4) as expected)
>
> Please simply ignore this if I'm talking rubbish :)
>
> Hope this helps.
>
> From a8831aadf57f39862560b37d507be2a3bcb99aa2 Mon Sep 17 00:00:00 2001
> From: Nacho Barrientos <nacho.barrientos@cern.ch>
> Date: Thu, 20 Oct 2022 14:16:43 +0200
> Subject: [PATCH] bindat (src, strz): operate on vectors too
>
> ---
>  lisp/emacs-lisp/bindat.el            |  4 ++--
>  test/lisp/emacs-lisp/bindat-tests.el | 19 ++++++++++++++++++-
>  2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
> index 0ecac3d52a..83188b369e 100644
> --- a/lisp/emacs-lisp/bindat.el
> +++ b/lisp/emacs-lisp/bindat.el
> @@ -163,7 +163,7 @@ bindat--unpack-str
>    (let ((s (substring bindat-raw bindat-idx (+ bindat-idx len))))
>      (setq bindat-idx (+ bindat-idx len))
>      (if (stringp s) s
> -      (apply #'unibyte-string s))))
> +      (apply #'unibyte-string (append s nil)))))
>
>  (defun bindat--unpack-strz (&optional len)
>    (let ((i 0) s)
> @@ -172,7 +172,7 @@ bindat--unpack-strz
>      (setq s (substring bindat-raw bindat-idx (+ bindat-idx i)))
>      (setq bindat-idx (+ bindat-idx (or len (1+ i))))
>      (if (stringp s) s
> -      (apply #'unibyte-string s))))
> +      (apply #'unibyte-string (append s nil)))))
>
>  (defun bindat--unpack-bits (len)
>    (let ((bits nil) (bnum (1- (* 8 len))) j m)
> diff --git a/test/lisp/emacs-lisp/bindat-tests.el b/test/lisp/emacs-lisp/bindat-tests.el
> index 0c03c51e2e..2abf714852 100644
> --- a/test/lisp/emacs-lisp/bindat-tests.el
> +++ b/test/lisp/emacs-lisp/bindat-tests.el
> @@ -252,7 +252,24 @@ bindat-test--str-strz-multibyte
>      (should (equal (bindat-unpack spec "abc\0") "abc"))
>      ;; Missing null terminator.
>      (should-error (bindat-unpack spec ""))
> -    (should-error (bindat-unpack spec "a"))))
> +    (should-error (bindat-unpack spec "a")))
> +
> +  (ert-deftest bindat-test--strz-array-unpack ()
> +    (should (equal (bindat-unpack spec [#x61 #x62 #x63 #x00]) "abc"))))
> +
> +(let ((spec (bindat-type str 3)))
> +  (ert-deftest bindat-test--str-simple-array-unpack ()
> +    (should (equal (bindat-unpack spec [#x61 #x62 #x63]) "abc"))))
> +
> +(let ((spec (bindat-type
> +              (first u8)
> +              (string str 3)
> +              (last uint 16))))
> +  (ert-deftest bindat-test--str-combined-array-unpack ()
> +    (let ((unpacked (bindat-unpack spec [#xff #x63 #x62 #x61 #xff #xff])))
> +      (should (equal (bindat-get-field unpacked 'string) "cba"))
> +      (should (equal (bindat-get-field unpacked 'first) (- (expt 2 8) 1)))
> +      (should (equal (bindat-get-field unpacked 'last) (- (expt 2 16) 1))))))
>
>  (let ((spec '((x strz 2))))
>    (ert-deftest bindat-test--strz-legacy-fixedlen-len ()
> --
> 2.38.0