Re: [Emacs-diffs] master 515afc9 6/6: Fix crash if user test munges hash table

[Pip Cet]

On Sun, Jul 21, 2019 at 3:14 AM Paul Eggert <address@hidden> wrote:
> diff --git a/src/alloc.c b/src/alloc.c
> index 09b3a4e..1718ce0 100644
> --- a/src/alloc.c
> +++ b/src/alloc.c
> @@ -5352,6 +5352,7 @@ purecopy_hash_table (struct Lisp_Hash_Table *table)
>    pure->count = table->count;
>    pure->next_free = table->next_free;
>    pure->purecopy = table->purecopy;
> +  eassert (!pure->mutable);
>    pure->rehash_threshold = table->rehash_threshold;
>    pure->rehash_size = table->rehash_size;
>    pure->key_and_value = purecopy (table->key_and_value);

I don't think this eassert () is completely safe.  pure_alloc will
return uninitialized memory if pure space has overflowed, so it's
possible the new table is marked as mutable.

> diff --git a/test/src/fns-tests.el b/test/src/fns-tests.el
> index 9d4ae4f..7d56da7 100644
> --- a/test/src/fns-tests.el
> +++ b/test/src/fns-tests.el
> @@ -846,4 +846,16 @@
>    (should (not (proper-list-p (make-bool-vector 0 nil))))
>    (should (not (proper-list-p (make-symbol "a")))))
>
> +(ert-deftest test-hash-function-that-mutates-hash-table ()
> +  (define-hash-table-test 'badeq 'eq 'bad-hash)
> +  (let ((h (make-hash-table :test 'badeq :size 1 :rehash-size 1)))
> +    (defun bad-hash (k)
> +      (if (eq k 100)
> +         (clrhash h))
> +      (sxhash-eq k))
> +    (should-error
> +     (dotimes (k 200)
> +       (puthash k k h)))
> +    (should (= 100 (hash-table-count h)))))
> +
>  (provide 'fns-tests)

Is it really necessary to cater to code such as this? I thought the
general line was that it was okay for bad Lisp code to crash Emacs in
exceptional circumstances, such as by building bad bytecode objects or
by doing silly things in a user-defined hash function...