Bindat can exhaust memory when unpacking to vector

[Petteri Hintsanen]

It appears that bindat-unpack can exhaust memory when it unpacks to a
vector.  Consider the following Bindat type expression:

  (require 'bindat)
  (defconst foo-bindat-spec
    (bindat-type
     (length uint 32)
     (data vec length)))

If you then, for example, eval this

  (bindat-unpack foo-bindat-spec [255 255 255 255 0 1 2 3 4 5])

it will result in (make-vector 4294967295 0), which makes Emacs hang.

Now it can be argued whether this is a problem or not.

If the data being unpacked cannot be trusted, then some kind of sanity
checking should be done to avoid things like above.  In my application
this is the case, so I embedded validation into the spec itself:

  (defconst foo-bindat-spec
    (bindat-type
     (length uint 32)
     (_ unit (when (> length 1234) (error "malicious input")))
     (data vec length)))

Which is somewhat messy, but it works.

I also played around with the idea of patching bindat.el itself to do
trivial checking against the input data size, like this:

diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
index 6f2af7f975b..c58f6c8f5c7 100644
--- a/lisp/emacs-lisp/bindat.el
+++ b/lisp/emacs-lisp/bindat.el
@@ -204,6 +204,9 @@ bindat--unpack-item
    ('str (bindat--unpack-str len))
    ('strz (bindat--unpack-strz len))
    ('vec
+    (when (> len (length bindat-raw))
+      (error "Vector length %d is greater than raw data length %d."
+             len (length bindat-raw)))
     (let ((v (make-vector len 0)) (vlen 1))
       (if (consp vectype)
          (setq vlen (nth 1 vectype)

but this feels way too intrusive.  Checking should be optional and
somehow programmable, perhaps a separate "checked vec" type?  (I don't
have any good, concrete ideas, sorry.)

Nonetheless, I think it would be prudent to mention this potential
pitfall in the Emacs Lisp manual so that users become aware of it.
I can write a texinfo patch if you agree.

[Side note: vec seems to be the only bindat type that suffers from this.
str (string) is another type that has a varying length, but it is
unpacked by `substring' which will implicitly guard against nonsensical
lengths.]

Thanks,
Petteri