GNU bug report logs - #55897
[PATCH] bindat (str, strz): Convert to unibyte when packing

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 55897 in the body.
You can then email your comments to 55897 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Richard Hansen <rhansen <at> rhansen.org>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] bindat (str, strz): Convert to unibyte when packing
Date: Sat, 11 Jun 2022 00:38:00 -0400

[Message part 1 (text/plain, inline)]

X-Debbugs-CC: monnier <at> iro.umontreal.ca

Two patches attached:

Patch 1:

    bindat (str, strz): Reject multibyte input strings

    * lisp/emacs-lisp/bindat.el (str) (strz): Signal an error if the user
    attempts to pack a multibyte string.
    * test/lisp/emacs-lisp/bindat-tests.el (str) (strz): Add tests.

Patch 2:

    bindat (str, strz): Convert to unibyte when packing

    * lisp/emacs-lisp/bindat.el (str) (strz): Allow callers to pack a
    multibyte string if it only contains ASCII and `eight-bit' characters.
    * doc/lispref/processes.texi (Bindat Types): Update documentation.
    * test/lisp/emacs-lisp/bindat-tests.el (str) (strz): Update tests.

[0001-bindat-str-strz-Reject-multibyte-input-strings.patch (text/x-patch, attachment)]

[0002-bindat-str-strz-Convert-to-unibyte-when-packing.patch (text/x-patch, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #8 received at 55897 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Richard Hansen <rhansen <at> rhansen.org>
Cc: 55897 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#55897: [PATCH] bindat (str,
 strz): Convert to unibyte when packing
Date: Sat, 11 Jun 2022 11:11:15 +0300

> Cc: monnier <at> iro.umontreal.ca
> Date: Sat, 11 Jun 2022 00:38:00 -0400
> From: Richard Hansen <rhansen <at> rhansen.org>
> 
>  (defun bindat--pack-str (len v)
> +  (if (multibyte-string-p v)
> +      (signal 'wrong-type-argument `(multibyte-string-p ,v)))

Isn't this too strict?  First, a string can be multibyte and
pure-ASCII:

  (let ((str (decode-coding-string "abcde" 'utf-8)))
    (multibyte-string-p str))
    => t

Shouldn't it be possible to use such strings here?

Furthermore, I think you said you wanted to extend bindat so it could
use multibyte string that contain ASCII and eight-bit characters?  If
so, this sounds like shooting ourselves in the foot?

>  (defun bindat--pack-str (len v)
> -  (if (multibyte-string-p v)
> -      (signal 'wrong-type-argument `(multibyte-string-p ,v)))
> -  (dotimes (i (min len (length v)))
> -    (aset bindat-raw (+ bindat-idx i) (aref v i)))
> -  (setq bindat-idx (+ bindat-idx len)))
> +  (let ((v (string-to-unibyte v)))
> +    (dotimes (i (min len (length v)))
> +      (aset bindat-raw (+ bindat-idx i) (aref v i)))
> +    (setq bindat-idx (+ bindat-idx len))))

And here you remove that error back?  Why does it make sense to
introduce an error message, only to remove it in the very next commit?
Please instead make a single change which incorporates both.

> --- a/test/lisp/emacs-lisp/bindat-tests.el
> +++ b/test/lisp/emacs-lisp/bindat-tests.el
> @@ -193,13 +193,15 @@ bindat-test--str-strz-multibyte
>    (dolist (spec (list (bindat-type str 2)
>                        (bindat-type strz 2)
>                        (bindat-type strz)))
> -    (should-error (bindat-pack spec (string-to-multibyte "x")))
> -    (should-error (bindat-pack spec (string-to-multibyte "\xff")))
> +    (should (equal (bindat-pack spec (string-to-multibyte "x")) "x\0"))
> +    (should (equal (bindat-pack spec (string-to-multibyte "\xff")) "\xff\0"))
>      (should-error (bindat-pack spec "💩"))
>      (should-error (bindat-pack spec "\N{U+ff}")))
>    (dolist (spec (list '((x str 2)) '((x strz 2))))
> -    (should-error (bindat-pack spec `((x . ,(string-to-multibyte "x")))))
> -    (should-error (bindat-pack spec `((x . ,(string-to-multibyte "\xff")))))
> +    (should (equal (bindat-pack spec `((x . ,(string-to-multibyte "x"))))
> +                   "x\0"))
> +    (should (equal (bindat-pack spec `((x . ,(string-to-multibyte "\xff"))))
> +                   "\xff\0"))
>      (should-error (bindat-pack spec '((x . "💩"))))
>      (should-error (bindat-pack spec '((x . "\N{U+ff}"))))))

Likewise here.

Thanks.

P.S. Please also mention the bug number in the log message of the next
version of the patch, since the number is now known.

Message #11 received at 55897 <at> debbugs.gnu.org (full text, mbox):

From: Richard Hansen <rhansen <at> rhansen.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 55897 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#55897: [PATCH] bindat (str, strz): Convert to unibyte when
 packing
Date: Sun, 12 Jun 2022 01:23:17 -0400

[Message part 1 (text/plain, inline)]

> Please instead make a single change which incorporates both.

Done; see attached.

> P.S. Please also mention the bug number in the log message of the next
> version of the patch, since the number is now known.

Done.

[v2-0001-bindat-str-strz-Reject-non-ASCII-non-eight-bit-ch.patch (text/x-patch, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #16 received at 55897-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Richard Hansen <rhansen <at> rhansen.org>
Cc: 55897-done <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#55897: [PATCH] bindat (str, strz): Convert to unibyte when
 packing
Date: Sun, 12 Jun 2022 10:00:42 +0300

> Date: Sun, 12 Jun 2022 01:23:17 -0400
> Cc: 55897 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
> From: Richard Hansen <rhansen <at> rhansen.org>
> 
> > Please instead make a single change which incorporates both.
> 
> Done; see attached.
> 
> > P.S. Please also mention the bug number in the log message of the next
> > version of the patch, since the number is now known.
> 
> Done.

Thanks, installed.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.