bug#28489: 27.0.50; eieio-persistent slot type validation should be a bit smarter

bug#28489: 27.0.50; eieio-persistent slot type validation should be a bit smarter

[Eric Abrahamsen]

EIEIO object slots can have a :type that looks like "(or thing1 thing2
...)", meaning, obviously, that the value of the slot is legal if it is
of any of those types.

`eieio-persistent-validate/fix-slot-value' makes a first pass over the
slot type specification, to see if any of the "things" are class
symbols. The checking is done by
`eieio-persistent-slot-type-is-class-p', which is able to handle an `or'
statement. The bug comes with:

((eq (car-safe type) 'or)
 ;; If type is a list, and is an or, it is possibly something
 ;; like (or null myclass), so check for that.
 (let ((ans nil))
   (dolist (subtype (cdr type))
     (setq ans (eieio-persistent-slot-type-is-class-p
		subtype)))
   ans))

In effect, only the last element of the `or' statement is checked, which
is obviously a bug. The Right Thing would probably be to return all the
valid class types in the list (with `seq-filter' or its equivalent), and
then change the rest of the code to accept that list.

Otherwise, the spec and value get passed to `cl-typep' directly, which
would successfully validate, except that "value" is still an object
construction form, not a constructed object.

It just seems like there's a lot of overlap between this and `cl-typep'.
And, coincidentally, I'm getting bug reports about the very slow
performance of `eieio-persistent-read'! I wonder if `cl-typep' could be
taught to handle some more of these cases.

The minimum fix seems to be to have
`eieio-persistent-slot-type-is-class-p' return a list of classes when
necessary. I can take a whack at a patch for that, if acceptable.

In GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.21)
 of 2017-09-17 built on clem

bug#28489: Acknowledgement (27.0.50; eieio-persistent slot type validation should be a bit smarter)

[Eric Abrahamsen]

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

Here's a basic patch that does the bare minimum, and seems to work.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: orclasstypes.diff --]
[-- Type: text/x-diff, Size: 2232 bytes --]

diff --git a/lisp/emacs-lisp/eieio-base.el b/lisp/emacs-lisp/eieio-base.el
index 6b39b4f262..361c1a4eb9 100644
--- a/lisp/emacs-lisp/eieio-base.el
+++ b/lisp/emacs-lisp/eieio-base.el
@@ -308,14 +308,6 @@ eieio-persistent-validate/fix-slot-value
 		       (= (length proposed-value) 1))
 		  nil)
 
-		  ;; We have a slot with a single object that can be
-		  ;; saved here.  Recurse and evaluate that
-		  ;; sub-object.
-		 ((and classtype (class-p classtype)
-		       (child-of-class-p (car proposed-value) classtype))
-		  (eieio-persistent-convert-list-to-object
-		   proposed-value))
-
 		 ;; List of object constructors.
 		 ((and (eq (car proposed-value) 'list)
 		       ;; 2nd item is a list.
@@ -346,6 +338,16 @@ eieio-persistent-validate/fix-slot-value
 			    objlist))
 		    ;; return the list of objects ... reversed.
 		    (nreverse objlist)))
+                 ;; We have a slot with a single object that can be
+		 ;; saved here.  Recurse and evaluate that
+		 ;; sub-object.
+		 ((and classtype
+                       (seq-some
+                        (lambda (c-type)
+		          (child-of-class-p (car proposed-value) c-type))
+                        (if (listp classtype) classtype (list classtype))))
+		  (eieio-persistent-convert-list-to-object
+		   proposed-value))
 		 (t
 		  proposed-value))))
 
@@ -359,7 +361,7 @@ eieio-persistent-validate/fix-slot-value
   )
 
 (defun eieio-persistent-slot-type-is-class-p (type)
-  "Return the class referred to in TYPE.
+  "Return the class or classes referred to in TYPE.
 If no class is referenced there, then return nil."
   (cond ((class-p type)
 	 ;; If the type is a class, then return it.
@@ -402,13 +404,9 @@ eieio-persistent-slot-type-is-class-p
 	       type))
 
 	((eq (car-safe type) 'or)
-	 ;; If type is a list, and is an or, it is possibly something
-	 ;; like (or null myclass), so check for that.
-	 (let ((ans nil))
-	   (dolist (subtype (cdr type))
-	     (setq ans (eieio-persistent-slot-type-is-class-p
-			subtype)))
-	   ans))
+	 ;; If type is a list, and is an or, it is possible that
+	 ;; multiple classes are acceptable, find them all.
+	 (seq-filter (lambda (elt) (class-p elt)) (cdr type)))
 
 	(t
 	 ;; No match, not a class.

bug#28489: Acknowledgement (27.0.50; eieio-persistent slot type validation should be a bit smarter)

[Noam Postavsky]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

>  	((eq (car-safe type) 'or)
> -	 ;; If type is a list, and is an or, it is possibly something
> -	 ;; like (or null myclass), so check for that.
> -	 (let ((ans nil))
> -	   (dolist (subtype (cdr type))
> -	     (setq ans (eieio-persistent-slot-type-is-class-p
> -			subtype)))
> -	   ans))
> +	 ;; If type is a list, and is an or, it is possible that
> +	 ;; multiple classes are acceptable, find them all.
> +	 (seq-filter (lambda (elt) (class-p elt)) (cdr type)))

You seem to have removed some recursion here, is that correct?  If yes,
probably something worth explaining in the commit message.

Minor nitpicks:
- The lambda could be replaced with just #'class-p.
- The indentation has a mix of tabs and spaces.

bug#28489: Acknowledgement (27.0.50; eieio-persistent slot type validation should be a bit smarter)

[Eric Abrahamsen]

On 09/26/17 20:05 PM, Noam Postavsky wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>>  	((eq (car-safe type) 'or)
>> -	 ;; If type is a list, and is an or, it is possibly something
>> -	 ;; like (or null myclass), so check for that.
>> -	 (let ((ans nil))
>> -	   (dolist (subtype (cdr type))
>> -	     (setq ans (eieio-persistent-slot-type-is-class-p
>> -			subtype)))
>> -	   ans))
>> +	 ;; If type is a list, and is an or, it is possible that
>> +	 ;; multiple classes are acceptable, find them all.
>> +	 (seq-filter (lambda (elt) (class-p elt)) (cdr type)))
>
> You seem to have removed some recursion here, is that correct?  If yes,
> probably something worth explaining in the commit message.
>
> Minor nitpicks:
> - The lambda could be replaced with just #'class-p.
> - The indentation has a mix of tabs and spaces.

That was a dumb mistake! My only excuse is that this was just a quick
code sketch for purposes of discussion. That's my story...

It should be:

(seq-filter #'eieio-persistent-slot-type-is-class-p (cdr type))

This whole section of code, `eieio-persistent-validate/fix-slot-value'
and its neighbors, feels very seat-of-the-pants to me. I wish `cl-typep'
could handle more of this work. But in the meantime this patch (or
something like it) would at least address the actual bug.

I don't think the tabs were my fault! What's Emacs policy on this?

bug#28489: Acknowledgement (27.0.50; eieio-persistent slot type validation should be a bit smarter)

[Noam Postavsky]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> It should be:
>
> (seq-filter #'eieio-persistent-slot-type-is-class-p (cdr type))

A right, that makes more sense.

> This whole section of code, `eieio-persistent-validate/fix-slot-value'
> and its neighbors, feels very seat-of-the-pants to me. I wish `cl-typep'
> could handle more of this work. But in the meantime this patch (or
> something like it) would at least address the actual bug.

Hmm, to be honest I can't quite make out what this function is actually
being used for.

> I don't think the tabs were my fault! What's Emacs policy on this?

I believe the policy is that new code should use spaces (although
sometimes people ignore this, it's not a big deal), but don't touch
lines just for the sake of changing the whitespace.

bug#28489: Acknowledgement (27.0.50; eieio-persistent slot type validation should be a bit smarter)

[Eric Abrahamsen]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

[...]

>> This whole section of code, `eieio-persistent-validate/fix-slot-value'
>> and its neighbors, feels very seat-of-the-pants to me. I wish `cl-typep'
>> could handle more of this work. But in the meantime this patch (or
>> something like it) would at least address the actual bug.
>
> Hmm, to be honest I can't quite make out what this function is actually
> being used for.

It's basically a first pass at slot validation that is specific to the
way eieio-persistent writes objects to disk. It can't write actual
objects, obviously, so it writes object construction forms. Before the
"real" validation happens in `eieio--perform-slot-validation', the
persistent read process needs to make sure the construction forms will
produce objects of the right class, then eval the forms.

So if you've got a slot that's supposed to be of type (list-of foo), the
persistence pre-validation looks at the value read from file, sees that
it's (list (foo ...) (foo ...)), evals those construction forms, and
then what the "real" validation ends up with is (cl-typep (list #<foo>
#<foo>) '(list-of 'foo)), which passes.

Essentially it is validating twice, both before and after the actual
objects are created. I don't have a very firm grasp of all the code
involved, but in principle I would prefer just to eval all object
construction forms regardless, and then let it blow up at "real"
validation time -- it was going to blow up anyway.

Another option would be to make use of `eieio-skip-typecheck', which, if
non-nil, could be interpreted as saying to skip all slot-related type
checking, including in persistent object read.

But again, my patch or something like it would be enough to get
everything working as advertised.

>> I don't think the tabs were my fault! What's Emacs policy on this?
>
> I believe the policy is that new code should use spaces (although
> sometimes people ignore this, it's not a big deal), but don't touch
> lines just for the sake of changing the whitespace.

Good to know, thanks. In lines of code I've added, indentation will be
done with spaces, but I suppose that's okay?

bug#28489: Acknowledgement (27.0.50; eieio-persistent slot type validation should be a bit smarter)

[Noam Postavsky]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Essentially it is validating twice, both before and after the actual
> objects are created. I don't have a very firm grasp of all the code
> involved, but in principle I would prefer just to eval all object
> construction forms regardless, and then let it blow up at "real"
> validation time -- it was going to blow up anyway.

Hmm, yeah, it does look the prevalidation is mostly redundant work.  The
docstring of eieio-persistent-convert-list-to-object mentions malicious
code, perhaps the prevalidation should be with unsafep (i.e., don't try
to typecheck anything, just make sure it's safe to eval).  This would
require that object constructors could be marked safe though.

> But again, my patch or something like it would be enough to get
> everything working as advertised.

Right.  I think your patch is probably fine, though a few tests might a
good idea too.

>>> I don't think the tabs were my fault! What's Emacs policy on this?
>>
>> I believe the policy is that new code should use spaces (although
>> sometimes people ignore this, it's not a big deal), but don't touch
>> lines just for the sake of changing the whitespace.
>
> Good to know, thanks. In lines of code I've added, indentation will be
> done with spaces, but I suppose that's okay?

Yes, sorry for being unclear, that's what I meant.  "new code" was
supposed to refer to lines that are added (or modified), as opposed to
"old code" being the unchanged lines.

bug#28489: Acknowledgement (27.0.50; eieio-persistent slot type validation should be a bit smarter)

[Eric Abrahamsen]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Essentially it is validating twice, both before and after the actual
>> objects are created. I don't have a very firm grasp of all the code
>> involved, but in principle I would prefer just to eval all object
>> construction forms regardless, and then let it blow up at "real"
>> validation time -- it was going to blow up anyway.
>
> Hmm, yeah, it does look the prevalidation is mostly redundant work.  The
> docstring of eieio-persistent-convert-list-to-object mentions malicious
> code, perhaps the prevalidation should be with unsafep (i.e., don't try
> to typecheck anything, just make sure it's safe to eval).  This would
> require that object constructors could be marked safe though.

That sounds like the right solution. I've never looked at
unsafep.el, and don't know exactly how it works, but in principle I
think object creation should be "safe". Of note:

1. Strings read from the persistence file are already stripped of
   properties.
2. Slot values are simply validated by type. I don't think eval is
   called anywhere, though I'd be happy to try to make sure of this.
3. Object creation could run malicious code *if* someone had overridden
   `initialize-instance' or `shared-initialize', and injected random
   hard-drive-destroying code. But I don't think this malicious code
   could be included in a persistence file itself (that's worth looking
   in to).

I don't know how close that gets us to "safe".

>> But again, my patch or something like it would be enough to get
>> everything working as advertised.
>
> Right.  I think your patch is probably fine, though a few tests might a
> good idea too.

I might as well write tests that exercise the whole eieio-persistent
round-trip: create a few test objects, write them to a tmp file, and
read them back as objects.

Thanks,
Eric

bug#28489: Acknowledgement (27.0.50; eieio-persistent slot type validation should be a bit smarter)

[Noam Postavsky]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> That sounds like the right solution. I've never looked at
> unsafep.el, and don't know exactly how it works,

Basically, there is a whitelist: symbols which have the property `safe',
are ok, stuff like progn is okay if all the things inside are also
`safe'.  So if we can be sure an object constructor does nothing but
create an object then it could be marked safe.

> 3. Object creation could run malicious code *if* someone had overridden
>    `initialize-instance' or `shared-initialize',

Hmm, it might be a difficult to be confident that calling some generic
function is safe.

> I might as well write tests that exercise the whole eieio-persistent
> round-trip: create a few test objects, write them to a tmp file, and
> read them back as objects.

Sounds good.

bug#28489: Acknowledgement (27.0.50; eieio-persistent slot type validation should be a bit smarter)

[Eric Abrahamsen]

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

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> That sounds like the right solution. I've never looked at
>> unsafep.el, and don't know exactly how it works,
>
> Basically, there is a whitelist: symbols which have the property `safe',
> are ok, stuff like progn is okay if all the things inside are also
> `safe'.  So if we can be sure an object constructor does nothing but
> create an object then it could be marked safe.
>
>> 3. Object creation could run malicious code *if* someone had overridden
>>    `initialize-instance' or `shared-initialize',
>
> Hmm, it might be a difficult to be confident that calling some generic
> function is safe.

It would indeed be complicated -- you'd have to locate all
currently-defined methods for those two generics, then run through them
and make sure everything in them was safe. I suppose it would be
possible, but pretty annoying.

I still think it would be worth bringing `eieio-skip-typecheck' into
play. I'm probably the only person in the world who cares about the
performance of eieio-persistent-read, but it wouldn't hurt!

>> I might as well write tests that exercise the whole eieio-persistent
>> round-trip: create a few test objects, write them to a tmp file, and
>> read them back as objects.
>
> Sounds good.

Here's the commit as it stands, seems to work fine. I'll let it mellow
for a while, and then commit to... emacs-26? Since it's technically a
bug fix?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-slot-typecheck-in-eieio-persistent.patch --]
[-- Type: text/x-diff, Size: 4117 bytes --]

From 45c2e7ffbbcbe564b816af85274ac84a170fca3e Mon Sep 17 00:00:00 2001
From: Eric Abrahamsen <eric@ericabrahamsen.net>
Date: Sat, 30 Sep 2017 10:57:52 -0700
Subject: [PATCH] Fix slot typecheck in eieio-persistent

* lisp/emacs-lisp/eieio-base.el (eieio-persistent-slot-type-is-class-p):
  An `or' form can specify multiple potential classes (or null) as
  valid types for a slot, but previously only the final element of the
  `or' was actually checked. Now returns all valid classes in the `or'
  form.
* lisp/emacs-lisp/eieio-base.el (eieio-persistent-validate/fix-slot-value):
  Check if proposed value matches any of the valid classes.
* test/lisp/emacs-lisp/eieio-tests/eieio-test-persist.el
  (eieio-test-multiple-class-slot): Test this behavior.
---
 lisp/emacs-lisp/eieio-base.el                      | 28 ++++++++++------------
 .../emacs-lisp/eieio-tests/eieio-test-persist.el   | 21 ++++++++++++++++
 2 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/lisp/emacs-lisp/eieio-base.el b/lisp/emacs-lisp/eieio-base.el
index 6b39b4f262..1ddf9c4460 100644
--- a/lisp/emacs-lisp/eieio-base.el
+++ b/lisp/emacs-lisp/eieio-base.el
@@ -308,14 +308,6 @@ eieio-persistent-validate/fix-slot-value
 		       (= (length proposed-value) 1))
 		  nil)
 
-		  ;; We have a slot with a single object that can be
-		  ;; saved here.  Recurse and evaluate that
-		  ;; sub-object.
-		 ((and classtype (class-p classtype)
-		       (child-of-class-p (car proposed-value) classtype))
-		  (eieio-persistent-convert-list-to-object
-		   proposed-value))
-
 		 ;; List of object constructors.
 		 ((and (eq (car proposed-value) 'list)
 		       ;; 2nd item is a list.
@@ -346,6 +338,16 @@ eieio-persistent-validate/fix-slot-value
 			    objlist))
 		    ;; return the list of objects ... reversed.
 		    (nreverse objlist)))
+		 ;; We have a slot with a single object that can be
+		 ;; saved here.  Recurse and evaluate that
+		 ;; sub-object.
+		 ((and classtype
+                       (seq-some
+                        (lambda (elt)
+                          (child-of-class-p (car proposed-value) elt))
+                        classtype))
+		  (eieio-persistent-convert-list-to-object
+		   proposed-value))
 		 (t
 		  proposed-value))))
 
@@ -402,13 +404,9 @@ eieio-persistent-slot-type-is-class-p
 	       type))
 
 	((eq (car-safe type) 'or)
-	 ;; If type is a list, and is an or, it is possibly something
-	 ;; like (or null myclass), so check for that.
-	 (let ((ans nil))
-	   (dolist (subtype (cdr type))
-	     (setq ans (eieio-persistent-slot-type-is-class-p
-			subtype)))
-	   ans))
+	 ;; If type is a list, and is an `or', return all valid class
+	 ;; types within the `or' statement.
+	 (seq-filter #'eieio-persistent-slot-type-is-class-p (cdr type)))
 
 	(t
 	 ;; No match, not a class.
diff --git a/test/lisp/emacs-lisp/eieio-tests/eieio-test-persist.el b/test/lisp/emacs-lisp/eieio-tests/eieio-test-persist.el
index e2cff3fbca..59eb287bc9 100644
--- a/test/lisp/emacs-lisp/eieio-tests/eieio-test-persist.el
+++ b/test/lisp/emacs-lisp/eieio-tests/eieio-test-persist.el
@@ -195,6 +195,27 @@ persistent-with-objs-slot-subs
     (persist-test-save-and-compare persist-woss)
     (delete-file (oref persist-woss file))))
 
+;; A slot that can contain one of two different classes, to exercise
+;; the `or' slot type.
+
+(defclass persistent-random-class ()
+  ())
+
+(defclass persistent-multiclass-slot (eieio-persistent)
+  ((slot1 :initarg :slot1
+          :type (or persistent-random-class null persist-not-persistent))
+   (slot2 :initarg :slot2
+          :type (or persist-not-persistent persist-random-class null))))
+
+(ert-deftest eieio-test-multiple-class-slot ()
+  (let ((persist
+         (persistent-multiclass-slot "random string"
+          :slot1 (persistent-random-class)
+          :slot2 (persist-not-persistent)
+          :file (concat default-directory "test-ps5.pt"))))
+    (persist-test-save-and-compare persist)
+    (delete-file (oref persist file))))
+
 ;;; Slot with a list of Objects
 ;;
 ;; A slot that contains another object that isn't persistent
-- 
2.14.2

bug#28489: Acknowledgement (27.0.50; eieio-persistent slot type validation should be a bit smarter)

[Noam Postavsky]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Here's the commit as it stands, seems to work fine. I'll let it mellow
> for a while, and then commit to... emacs-26? Since it's technically a
> bug fix?

Yes.  You are missing a (require 'seq) in eieo-base.el though (without
that, I get an error when running 'make -C test eieio-test-persist').

> * lisp/emacs-lisp/eieio-base.el (eieio-persistent-slot-type-is-class-p):
>   An `or' form can specify multiple potential classes (or null) as
>   valid types for a slot, but previously only the final element of the
>   `or' was actually checked. Now returns all valid classes in the `or'
>   form.
> * lisp/emacs-lisp/eieio-base.el (eieio-persistent-validate/fix-slot-value):
>   Check if proposed value matches any of the valid classes.

These 2 entries should be combined like:

* lisp/emacs-lisp/eieio-base.el (eieio-persistent-slot-type-is-class-p):
An `or' form can specify multiple potential classes (or null) as [...]
(eieio-persistent-validate/fix-slot-value): Check if proposed value
matches any of the valid classes.

> +(ert-deftest eieio-test-multiple-class-slot ()
> +  (let ((persist
> +         (persistent-multiclass-slot "random string"
> +          :slot1 (persistent-random-class)
> +          :slot2 (persist-not-persistent)
> +          :file (concat default-directory "test-ps5.pt"))))
> +    (persist-test-save-and-compare persist)
> +    (delete-file (oref persist file))))

I think this test should a bit more thorough about deleting the file,
even if the test fails for some reason:

(unwind-protect
    (persist-test-save-and-compare persist)
  (ignore-errors (delete-file (oref persist file))))

bug#28489: Acknowledgement (27.0.50; eieio-persistent slot type validation should be a bit smarter)

[Eric Abrahamsen]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Here's the commit as it stands, seems to work fine. I'll let it mellow
>> for a while, and then commit to... emacs-26? Since it's technically a
>> bug fix?
>
> Yes.  You are missing a (require 'seq) in eieo-base.el though (without
> that, I get an error when running 'make -C test eieio-test-persist').

Thanks, done.

>> * lisp/emacs-lisp/eieio-base.el (eieio-persistent-slot-type-is-class-p):
>>   An `or' form can specify multiple potential classes (or null) as
>>   valid types for a slot, but previously only the final element of the
>>   `or' was actually checked. Now returns all valid classes in the `or'
>>   form.
>> * lisp/emacs-lisp/eieio-base.el (eieio-persistent-validate/fix-slot-value):
>>   Check if proposed value matches any of the valid classes.
>
> These 2 entries should be combined like:
>
> * lisp/emacs-lisp/eieio-base.el (eieio-persistent-slot-type-is-class-p):
> An `or' form can specify multiple potential classes (or null) as [...]
> (eieio-persistent-validate/fix-slot-value): Check if proposed value
> matches any of the valid classes.

Dunno how that happened...

>> +(ert-deftest eieio-test-multiple-class-slot ()
>> +  (let ((persist
>> +         (persistent-multiclass-slot "random string"
>> +          :slot1 (persistent-random-class)
>> +          :slot2 (persist-not-persistent)
>> +          :file (concat default-directory "test-ps5.pt"))))
>> +    (persist-test-save-and-compare persist)
>> +    (delete-file (oref persist file))))
>
> I think this test should a bit more thorough about deleting the file,
> even if the test fails for some reason:
>
> (unwind-protect
>     (persist-test-save-and-compare persist)
>   (ignore-errors (delete-file (oref persist file))))

Done -- at some point, the same should be done for the other tests.

Thanks,
Eric

bug#28489: Acknowledgement (27.0.50; eieio-persistent slot type validation should be a bit smarter)

[Eric Abrahamsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> On 09/28/17 20:35 PM, Noam Postavsky wrote:
>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>
>>> Essentially it is validating twice, both before and after the actual
>>> objects are created. I don't have a very firm grasp of all the code
>>> involved, but in principle I would prefer just to eval all object
>>> construction forms regardless, and then let it blow up at "real"
>>> validation time -- it was going to blow up anyway.
>>
>> Hmm, yeah, it does look the prevalidation is mostly redundant work.  The
>> docstring of eieio-persistent-convert-list-to-object mentions malicious
>> code, perhaps the prevalidation should be with unsafep (i.e., don't try
>> to typecheck anything, just make sure it's safe to eval).  This would
>> require that object constructors could be marked safe though.
>
> I've never looked at `unsafep' and don't know what's involved in marking
> object constructors as safe, but that certainly sounds like the right
> solution.
>
> Object creation *ought to* be safe. First, properties are already
> stripped from strings. Second, the only way a creation of an object
> could have side effects is if someone overloaded `initialize-instance'
> or `shared-initialize' and inserted random hard-drive-destroying code
> there. But `eieio-persistent-read' can't do that by itself; it would
> have to be run in conjunction with a separate malicious library.
>
> Otherwise, object creation really just involves making objects,
> validating the data that 
>
> Aga
>
>>> But again, my patch or something like it would be enough to get
>>> everything working as advertised.
>>
>> Right.  I think your patch is probably fine, though a few tests might a
>> good idea too.
>
> Tests are an excellent idea. Why don't I fool with this patch a bit
> longer, write some tests, and commit the smallest change possible. Then
> open another bug report on the larger question of validation, and the
> possibility of marking object constructors as safe.