GNU bug report logs - #46534
Lexical change in bindat breaks weechat.el

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 46534 in the body.
You can then email your comments to 46534 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: Tony Olagbaiye <me <at> fron.io>
To: bug-gnu-emacs <at> gnu.org
Cc: monnier <monnier <at> iro.umontreal.ca>
Subject: Lexical change in bindat breaks weechat.el
Date: Mon, 15 Feb 2021 14:29:06 +0000

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

Hi,

As of commit c8c4d65d6510724acd40527a9af67e21e3cf4d5e (as bisected in my stead by wasamasa on \#emacs) it seems bindat changes have broken weechat.el, or specifically the weechat-relay module.

Attached a minimal reproducible script which fails on master but succeeds prior to the mentioned commit: weechat-bindat.bug.el.txt

Backtrace:

Debugger entered--Lisp error: (void-variable struct) (bindat-get-field struct 'len) (weechat--relay-bindat-unsigned-to-signed (bindat-get-field struct 'len) 4) (let ((len (weechat--relay-bindat-unsigned-to-signed (bindat-get-field struct 'len) 4))) (if (<= len 0) 0 len)) bindat--unpack-group(((len u32) (val str (eval (let ((len (weechat--relay-bindat-unsigned-to-signed ... 4))) (if (<= len 0) 0 len)))))) bindat--unpack-group(((length u32) (compression u8) (id struct weechat--relay-str-spec) (data vec (eval (let ((l (- ... 4 1 ...))) l))))) bindat-unpack(((length u32) (compression u8) (id struct weechat--relay-str-spec) (data vec (eval (let ((l (- ... 4 1 ...))) l)))) "\\0\\0\\0$\\0\\0\\0\\0\\4G255inf\\0\\0\\0\\7version\\0\\0\\0\\0053.0.1") weechat-unpack-message("\\0\\0\\0$\\0\\0\\0\\0\\4G255inf\\0\\0\\0\\7version\\0\\0\\0\\0053.0.1") weechat--relay-parse-new-message() weechat--relay-process-filter(\#<process weechat-relay-tls> "\\0\\0\\0$\\0\\0\\0\\0\\4G255inf\\0\\0\\0\\7version\\0\\0\\0\\0053.0.1")

Best,
bqv

[Message part 2 (text/html, inline)]

[weechat-bindat.bug.el.txt (text/plain, attachment)]

[publickey - EmailAddress(s=me@fron.io) - 0x3026807C.asc (application/pgp-keys, attachment)]

[signature.asc (application/pgp-signature, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Tony Olagbaiye <me <at> fron.io>
Cc: 46534 <at> debbugs.gnu.org, "Kim F. Storm" <storm <at> cua.dk>
Subject: bug#46534: Lexical change in bindat breaks weechat.el
Date: Mon, 15 Feb 2021 10:50:30 -0500

[ Hi Kim, long time no see.
  I'd appreciate your opinion on this issue with bindat.el.  ]

> (defconst minrepro--str-spec
>   '((len u32)
>     (val str (eval (let ((len (minrepro--bindat-unsigned-to-signed
> 			       (bindat-get-field struct 'len)
> 			       4)))
> 		     ;; Hack for signed/unsigned problems
> 		     (if (<= len 0) 0 len))))))

Hmm... the doc of bindat.el does not include `struct` among the vars you
can use in `eval`.

OTOH, a variable which you can use is `last` and it indeed contains
exactly the info you need from `struct`, so you can rewrite the above to:

    (defconst minrepro--str-spec
      '((len u32)
        (val str (eval (let ((len (minrepro--bindat-unsigned-to-signed
                                   last 4)))
                       ;; Hack for signed/unsigned problems
                       (if (<= len 0) 0 len))))))

> (defconst minrepro--message-spec
>   '((length u32)
>     (compression u8)
>     (id struct minrepro--str-spec)
>     (data vec (eval (let ((l (- (bindat-get-field struct 'length)
> 				4   ;length
> 				1   ;compression
> 				(+ 4 (length (bindat-get-field struct 'id 'val))))))
> 		      l)))))

This one OTOH can't just use `last` since that only gives us the `id`
field but not the `length` field  :-(

I can't see any way to do what you want given the documentation found in
the `Commentary:` of `bindat.el`, so I guess we do need to extend the
documented functionality.

I installed the patch below, for now.  It fixes the problem in your test
case and hopefully in other cases as well.  Please confirm.


        Stefan


diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index 0d9ba57d66..bf01347ae0 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -26,7 +26,7 @@
 ;;  Packing and unpacking of (binary) data structures.
 ;;
 ;;  The data formats used in binary files and network protocols are
-;;  often structed data which can be described by a C-style structure
+;;  often structured data which can be described by a C-style structure
 ;;  such as the one shown below.  Using the bindat package, decoding
 ;;  and encoding binary data formats like these is made simple using a
 ;;  structure specification which closely resembles the C style
@@ -135,7 +135,8 @@
 ;;          |  ( [FIELD] repeat COUNT ITEM... )
 
 ;;          -- In (eval EXPR), the value of the last field is available in
-;;             the dynamically bound variable `last'.
+;;             the dynamically bound variable `last' and all the previous
+;;             ones in the variable `struct'.
 
 ;; TYPE    ::= ( eval EXPR )		-- interpret result as TYPE
 ;;	    |  u8   | byte		-- length 1
@@ -191,7 +192,7 @@
 ;;; Code:
 
 ;; Helper functions for structure unpacking.
-;; Relies on dynamic binding of BINDAT-RAW and BINDAT-IDX
+;; Relies on dynamic binding of `bindat-raw' and `bindat-idx'.
 
 (defvar bindat-raw)
 (defvar bindat-idx)
@@ -276,8 +277,8 @@ bindat--unpack-item
    (t nil)))
 
 (defun bindat--unpack-group (spec)
-  (with-suppressed-warnings ((lexical last))
-    (defvar last))
+  (with-suppressed-warnings ((lexical struct last))
+    (defvar struct) (defvar last))
   (let (struct last)
     (while spec
       (let* ((item (car spec))
@@ -378,9 +379,9 @@ bindat--fixed-length-alist
     (ip . 4)))
 
 (defun bindat--length-group (struct spec)
-  (with-suppressed-warnings ((lexical last))
-    (defvar last))
-  (let (last)
+  (with-suppressed-warnings ((lexical struct last))
+    (defvar struct) (defvar last))
+  (let ((struct struct) last)
     (while spec
       (let* ((item (car spec))
 	     (field (car item))
@@ -544,9 +545,9 @@ bindat--pack-item
     (setq bindat-idx (+ bindat-idx len)))))
 
 (defun bindat--pack-group (struct spec)
-  (with-suppressed-warnings ((lexical last))
-    (defvar last))
-  (let (last)
+  (with-suppressed-warnings ((lexical struct last))
+    (defvar struct) (defvar last))
+  (let ((struct struct) last)
     (while spec
       (let* ((item (car spec))
 	     (field (car item))

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

From: Tony Olagbaiye <me <at> fron.io>
To: monnier <at> iro.umontreal.ca
Cc: 46534 <at> debbugs.gnu.org, storm <at> cua.dk
Subject: Re: bug#46534: Lexical change in bindat breaks weechat.el
Date: Mon, 15 Feb 2021 19:19:54 +0000

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

Indeed, I can confirm this fixes the test file as well as weechat.el proper, thanks!


Sent from ProtonMail mobile



\-------- Original Message --------
On 15 Feb 2021, 15:50, Stefan Monnier < monnier <at> iro.umontreal.ca> wrote:

>
>
>
> \[ Hi Kim, long time no see.
> I'd appreciate your opinion on this issue with bindat.el. \]
>
> > (defconst minrepro--str-spec
> > '((len u32)
> > (val str (eval (let ((len (minrepro--bindat-unsigned-to-signed
> > (bindat-get-field struct 'len)
> > 4)))
> > ;; Hack for signed/unsigned problems
> > (if (<= len 0) 0 len))))))
>
> Hmm... the doc of bindat.el does not include \`struct\` among the vars you
> can use in \`eval\`.
>
> OTOH, a variable which you can use is \`last\` and it indeed contains
> exactly the info you need from \`struct\`, so you can rewrite the above to:
>
> (defconst minrepro--str-spec
> '((len u32)
> (val str (eval (let ((len (minrepro--bindat-unsigned-to-signed
> last 4)))
> ;; Hack for signed/unsigned problems
> (if (<= len 0) 0 len))))))
>
> > (defconst minrepro--message-spec
> > '((length u32)
> > (compression u8)
> > (id struct minrepro--str-spec)
> > (data vec (eval (let ((l (- (bindat-get-field struct 'length)
> > 4 ;length
> > 1 ;compression
> > (+ 4 (length (bindat-get-field struct 'id 'val))))))
> > l)))))
>
> This one OTOH can't just use \`last\` since that only gives us the \`id\`
> field but not the \`length\` field :-(
>
> I can't see any way to do what you want given the documentation found in
> the \`Commentary:\` of \`bindat.el\`, so I guess we do need to extend the
> documented functionality.
>
> I installed the patch below, for now. It fixes the problem in your test
> case and hopefully in other cases as well. Please confirm.
>
>
> Stefan
>
>
> diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
> index 0d9ba57d66..bf01347ae0 100644
> \--- a/lisp/emacs-lisp/bindat.el
> \+++ b/lisp/emacs-lisp/bindat.el
> @@ -26,7 +26,7 @@
> ;; Packing and unpacking of (binary) data structures.
> ;;
> ;; The data formats used in binary files and network protocols are
> \-;; often structed data which can be described by a C-style structure
> \+;; often structured data which can be described by a C-style structure
> ;; such as the one shown below. Using the bindat package, decoding
> ;; and encoding binary data formats like these is made simple using a
> ;; structure specification which closely resembles the C style
> @@ -135,7 +135,8 @@
> ;; \| ( \[FIELD\] repeat COUNT ITEM... )
>
> ;; -- In (eval EXPR), the value of the last field is available in
> \-;; the dynamically bound variable \`last'.
> \+;; the dynamically bound variable \`last' and all the previous
> \+;; ones in the variable \`struct'.
>
> ;; TYPE ::= ( eval EXPR ) -- interpret result as TYPE
> ;; \| u8 \| byte -- length 1
> @@ -191,7 +192,7 @@
> ;;; Code:
>
> ;; Helper functions for structure unpacking.
> \-;; Relies on dynamic binding of BINDAT-RAW and BINDAT-IDX
> \+;; Relies on dynamic binding of \`bindat-raw' and \`bindat-idx'.
>
> (defvar bindat-raw)
> (defvar bindat-idx)
> @@ -276,8 +277,8 @@ bindat--unpack-item
> (t nil)))
>
> (defun bindat--unpack-group (spec)
> \- (with-suppressed-warnings ((lexical last))
> \- (defvar last))
> \+ (with-suppressed-warnings ((lexical struct last))
> \+ (defvar struct) (defvar last))
> (let (struct last)
> (while spec
> (let\* ((item (car spec))
> @@ -378,9 +379,9 @@ bindat--fixed-length-alist
> (ip . 4)))
>
> (defun bindat--length-group (struct spec)
> \- (with-suppressed-warnings ((lexical last))
> \- (defvar last))
> \- (let (last)
> \+ (with-suppressed-warnings ((lexical struct last))
> \+ (defvar struct) (defvar last))
> \+ (let ((struct struct) last)
> (while spec
> (let\* ((item (car spec))
> (field (car item))
> @@ -544,9 +545,9 @@ bindat--pack-item
> (setq bindat-idx (+ bindat-idx len)))))
>
> (defun bindat--pack-group (struct spec)
> \- (with-suppressed-warnings ((lexical last))
> \- (defvar last))
> \- (let (last)
> \+ (with-suppressed-warnings ((lexical struct last))
> \+ (defvar struct) (defvar last))
> \+ (let ((struct struct) last)
> (while spec
> (let\* ((item (car spec))
> (field (car item))

[Message part 2 (text/html, inline)]

[publickey - EmailAddress(s=me@fron.io) - 0x3026807C.asc (application/pgp-keys, attachment)]

[signature.asc (application/pgp-signature, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Tony Olagbaiye <me <at> fron.io>
Cc: storm <at> cua.dk, 46534-done <at> debbugs.gnu.org
Subject: Re: bug#46534: Lexical change in bindat breaks weechat.el
Date: Mon, 15 Feb 2021 14:51:29 -0500

> Indeed, I can confirm this fixes the test file as well as weechat.el proper, thanks!

Thanks, closing for now, tho I'd love to see a better fix
(e.g. changing the system so it doesn't rely on `eval` and dynamic
scoping so much).


        Stefan


> Sent from ProtonMail mobile
>
> -------- Original Message --------
> On 15 Feb 2021, 15:50, Stefan Monnier < monnier <at> iro.umontreal.ca> wrote:
>
>  [ Hi Kim, long time no see.
>  I'd appreciate your opinion on this issue with bindat.el. ]
>
>  > (defconst minrepro--str-spec
>  > '((len u32)
>  > (val str (eval (let ((len (minrepro--bindat-unsigned-to-signed
>  > (bindat-get-field struct 'len)
>  > 4)))
>  > ;; Hack for signed/unsigned problems
>  > (if (<= len 0) 0 len))))))
>
>  Hmm... the doc of bindat.el does not include `struct` among the vars you
>  can use in `eval`.
>
>  OTOH, a variable which you can use is `last` and it indeed contains
>  exactly the info you need from `struct`, so you can rewrite the above to:
>
>  (defconst minrepro--str-spec
>  '((len u32)
>  (val str (eval (let ((len (minrepro--bindat-unsigned-to-signed
>  last 4)))
>  ;; Hack for signed/unsigned problems
>  (if (<= len 0) 0 len))))))
>
>  > (defconst minrepro--message-spec
>  > '((length u32)
>  > (compression u8)
>  > (id struct minrepro--str-spec)
>  > (data vec (eval (let ((l (- (bindat-get-field struct 'length)
>  > 4 ;length
>  > 1 ;compression
>  > (+ 4 (length (bindat-get-field struct 'id 'val))))))
>  > l)))))
>
>  This one OTOH can't just use `last` since that only gives us the `id`
>  field but not the `length` field :-(
>
>  I can't see any way to do what you want given the documentation found in
>  the `Commentary:` of `bindat.el`, so I guess we do need to extend the
>  documented functionality.
>
>  I installed the patch below, for now. It fixes the problem in your test
>  case and hopefully in other cases as well. Please confirm.
>
>  Stefan
>
>  diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
>  index 0d9ba57d66..bf01347ae0 100644
>  --- a/lisp/emacs-lisp/bindat.el
>  +++ b/lisp/emacs-lisp/bindat.el
>  @@ -26,7 +26,7 @@
>  ;; Packing and unpacking of (binary) data structures.
>  ;;
>  ;; The data formats used in binary files and network protocols are
>  -;; often structed data which can be described by a C-style structure
>  +;; often structured data which can be described by a C-style structure
>  ;; such as the one shown below. Using the bindat package, decoding
>  ;; and encoding binary data formats like these is made simple using a
>  ;; structure specification which closely resembles the C style
>  @@ -135,7 +135,8 @@
>  ;; | ( [FIELD] repeat COUNT ITEM... )
>
>  ;; -- In (eval EXPR), the value of the last field is available in
>  -;; the dynamically bound variable `last'.
>  +;; the dynamically bound variable `last' and all the previous
>  +;; ones in the variable `struct'.
>
>  ;; TYPE ::= ( eval EXPR ) -- interpret result as TYPE
>  ;; | u8 | byte -- length 1
>  @@ -191,7 +192,7 @@
>  ;;; Code:
>
>  ;; Helper functions for structure unpacking.
>  -;; Relies on dynamic binding of BINDAT-RAW and BINDAT-IDX
>  +;; Relies on dynamic binding of `bindat-raw' and `bindat-idx'.
>
>  (defvar bindat-raw)
>  (defvar bindat-idx)
>  @@ -276,8 +277,8 @@ bindat--unpack-item
>  (t nil)))
>
>  (defun bindat--unpack-group (spec)
>  - (with-suppressed-warnings ((lexical last))
>  - (defvar last))
>  + (with-suppressed-warnings ((lexical struct last))
>  + (defvar struct) (defvar last))
>  (let (struct last)
>  (while spec
>  (let* ((item (car spec))
>  @@ -378,9 +379,9 @@ bindat--fixed-length-alist
>  (ip . 4)))
>
>  (defun bindat--length-group (struct spec)
>  - (with-suppressed-warnings ((lexical last))
>  - (defvar last))
>  - (let (last)
>  + (with-suppressed-warnings ((lexical struct last))
>  + (defvar struct) (defvar last))
>  + (let ((struct struct) last)
>  (while spec
>  (let* ((item (car spec))
>  (field (car item))
>  @@ -544,9 +545,9 @@ bindat--pack-item
>  (setq bindat-idx (+ bindat-idx len)))))
>
>  (defun bindat--pack-group (struct spec)
>  - (with-suppressed-warnings ((lexical last))
>  - (defvar last))
>  - (let (last)
>  + (with-suppressed-warnings ((lexical struct last))
>  + (defvar struct) (defvar last))
>  + (let ((struct struct) last)
>  (while spec
>  (let* ((item (car spec))
>  (field (car item))

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

From: Kim Storm <storm <at> cua.dk>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>, Tony Olagbaiye <me <at> fron.io>
Cc: 46534 <at> debbugs.gnu.org
Subject: Re: bug#46534: Lexical change in bindat breaks weechat.el
Date: Wed, 17 Feb 2021 23:47:21 +0100

On 15/02/2021 16.50, Stefan Monnier wrote:
> [ Hi Kim, long time no see.
>    I'd appreciate your opinion on this issue with bindat.el.  ]

Hi Stefan

Indeed, it's been a while since I wrote that code :-)

Your change seems to be the simplest way to solve the "not last field" 
issue with "eval".

BTW, the following line seems wrong:

;;    (length       u16r)  ;; little endian order

Since the C struct specifies "unsigned long", it should be u32r rather 
than u16r.

Also the C code samples should use uint8_t uint16_t and uint32_t for 
clarity.

Kim

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Kim Storm <storm <at> cua.dk>
Cc: 46534 <at> debbugs.gnu.org, Tony Olagbaiye <me <at> fron.io>
Subject: Re: bug#46534: Lexical change in bindat breaks weechat.el
Date: Thu, 18 Feb 2021 11:09:53 -0500

>> [ Hi Kim, long time no see.
>>    I'd appreciate your opinion on this issue with bindat.el.  ]
> Indeed, it's been a while since I wrote that code :-)

As you can see, it's still alive and kicking.

> Your change seems to be the simplest way to solve the "not last field"
> issue with "eval".

Thanks for confirming.

> BTW, the following line seems wrong:
>
> ;;    (length       u16r)  ;; little endian order
>
> Since the C struct specifies "unsigned long", it should be u32r rather
> than u16r.

Good catch.

> Also the C code samples should use uint8_t uint16_t and uint32_t
>  for clarity.

Oh, yes, thanks,


        Stefan

Previous Next

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