Critical bytecode bug with hash tables while dumping emacs.

Critical bytecode bug with hash tables while dumping emacs.

[Vibhav Pant]

[-- Attachment #1: Type: text/plain, Size: 727 bytes --]

While, working on `byte-switch`, I discovered a critical bug when a
hash table is present in the constant vector of a bytecode function.
If such a function is called when temacs dumps a binary during the
biuld process, all keys and their corresponding values change to that
of a different hash table. The following patch demonstrates this by
testing the value of 'foo` in a table mapping 'foo to 'bar, and
erroring out if the test fails. This causes temacs to error out,
failing the build.

Since the current elisp codebase, doesn't use printed representation
anywhere to represent hash tables, this bug doesn't come up. However,
this may fail builds in the future, if future code does that.

-- 
Vibhav Pant
vibhavp@gmail.com

[-- Attachment #2: wrong-hash-table.patch --]
[-- Type: text/x-patch, Size: 683 bytes --]

diff --git a/lisp/custom.el b/lisp/custom.el
index 70b6839db3..49330bb1a4 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -146,6 +146,10 @@ custom-declare-variable
 set to nil, as the value is no longer rogue."
   (put symbol 'standard-value (purecopy (list default)))
   ;; Maybe this option was rogue in an earlier version.  It no longer is.
+  (let ((ht #s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8 data
+                          (foo bar))))
+    (unless (eq 'bar (gethash 'foo ht))
+      (error (format "This shouldn't be happening. Hash table: %s" ht))))
   (when (get symbol 'force-value)
     (put symbol 'force-value nil))
   (if (keywordp doc)

Re: Critical bytecode bug with hash tables while dumping emacs.

[Paul Eggert]

On 01/26/2017 05:03 AM, Vibhav Pant wrote:
> Since the current elisp codebase, doesn't use printed representation
> anywhere to represent hash tables, this bug doesn't come up.

In that case the bug is not critical, right? One way to address the 
problem is to say that code should not print hash tables before dumping. 
Admittedly this is a weird restriction that we should remove, but it's 
not urgent and we're planning to redo dumping anyway and can address 
this problem (if it still occurs) then.

Re: Critical bytecode bug with hash tables while dumping emacs.

[Vibhav Pant]

On Thu, Jan 26, 2017 at 11:03 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> In that case the bug is not critical, right? One way to address the problem
> is to say that code should not print hash tables before dumping.

It's critical in the sense that any code loaded from loadup.el is effectively
prohibited from using printed hash tables in any way. I've recently been
working on adding a new 'switch` bytecode op (@ branch feature/byte-switch),
which uses hash tables generated during compile time (so the constant vector
stores printed hash tables) as jump tables. This bug breaks switch entirely.

> we're planning to redo dumping anyway and can address this problem (if it still
> occurs) then.

I suspect this bug is related to purecopy, which I suppose isn't a part of the
redo (or is it? I don't know much about the new dumping code). If anyone has any
ideas about this issue, I'd appreciate some pointers on where to start.

Thanks,
Vibhav
-- 
Vibhav Pant
vibhavp@gmail.com

Re: Critical bytecode bug with hash tables while dumping emacs.

[Vibhav Pant]

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

Also, this one line patch crashes temacs with a SIGSEGV - the hash table
contents
are possibly getting corrupted because of a bad memory write.

On Fri, Jan 27, 2017 at 12:27 AM Vibhav Pant <vibhavp@gmail.com> wrote:

> On Thu, Jan 26, 2017 at 11:03 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> > In that case the bug is not critical, right? One way to address the
> problem
> > is to say that code should not print hash tables before dumping.
>
> It's critical in the sense that any code loaded from loadup.el is
> effectively
> prohibited from using printed hash tables in any way. I've recently been
> working on adding a new 'switch` bytecode op (@ branch
> feature/byte-switch),
> which uses hash tables generated during compile time (so the constant
> vector
> stores printed hash tables) as jump tables. This bug breaks switch
> entirely.
>
> > we're planning to redo dumping anyway and can address this problem (if
> it still
> > occurs) then.
>
> I suspect this bug is related to purecopy, which I suppose isn't a part of
> the
> redo (or is it? I don't know much about the new dumping code). If anyone
> has any
> ideas about this issue, I'd appreciate some pointers on where to start.
>
> Thanks,
> Vibhav
> --
> Vibhav Pant
> vibhavp@gmail.com
>

[-- Attachment #1.2: Type: text/html, Size: 2160 bytes --]

[-- Attachment #2: wrong-hash-table.patch --]
[-- Type: text/x-patch, Size: 672 bytes --]

diff --git a/lisp/custom.el b/lisp/custom.el
index 70b6839db3..beccb2b545 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -145,6 +145,8 @@ custom-declare-variable
 `standard-value'.  At the same time, SYMBOL's property `force-value' is
 set to nil, as the value is no longer rogue."
   (put symbol 'standard-value (purecopy (list default)))
+  (message "%s" #s(hash-table size 65 test eql rehash-size 1.5 rehash-threshold 0.8 data
+                              (1 2 3 4 5 6 7 8 9 1 a a b b c c d d e e q q w w r r t t)))
   ;; Maybe this option was rogue in an earlier version.  It no longer is.
   (when (get symbol 'force-value)
     (put symbol 'force-value nil))