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

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 28489 in the body.
You can then email your comments to 28489 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: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; eieio-persistent slot type validation should be a bit smarter
Date: Sun, 17 Sep 2017 17:43:58 -0700

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

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

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: 28489 <at> debbugs.gnu.org
Subject: Re: bug#28489: Acknowledgement (27.0.50;
 eieio-persistent slot type validation should be a bit smarter)
Date: Tue, 26 Sep 2017 13:22:50 -0700

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

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

[orclasstypes.diff (text/x-diff, attachment)]

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

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: 28489 <at> debbugs.gnu.org
Subject: Re: bug#28489: Acknowledgement (27.0.50;
 eieio-persistent slot type validation should be a bit smarter)
Date: Tue, 26 Sep 2017 20:05:25 -0400

Eric Abrahamsen <eric <at> 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.

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

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: Noam Postavsky <npostavs <at> users.sourceforge.net>
Cc: 28489 <at> debbugs.gnu.org
Subject: Re: bug#28489: Acknowledgement (27.0.50;
 eieio-persistent slot type validation should be a bit smarter)
Date: Wed, 27 Sep 2017 09:39:07 -0700

On 09/26/17 20:05 PM, Noam Postavsky wrote:
> Eric Abrahamsen <eric <at> 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?

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

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: 28489 <at> debbugs.gnu.org
Subject: Re: bug#28489: Acknowledgement (27.0.50;
 eieio-persistent slot type validation should be a bit smarter)
Date: Wed, 27 Sep 2017 22:23:14 -0400

Eric Abrahamsen <eric <at> 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.

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

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#28489: Acknowledgement (27.0.50;
 eieio-persistent slot type validation should be a bit smarter)
Date: Wed, 27 Sep 2017 22:02:58 -0700

Noam Postavsky <npostavs <at> 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?

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

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: 28489 <at> debbugs.gnu.org
Subject: Re: bug#28489: Acknowledgement (27.0.50;
 eieio-persistent slot type validation should be a bit smarter)
Date: Thu, 28 Sep 2017 20:35:13 -0400

Eric Abrahamsen <eric <at> 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.

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

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#28489: Acknowledgement (27.0.50;
 eieio-persistent slot type validation should be a bit smarter)
Date: Fri, 29 Sep 2017 13:31:59 -0700

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

> Eric Abrahamsen <eric <at> 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

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

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: 28489 <at> debbugs.gnu.org
Subject: Re: bug#28489: Acknowledgement (27.0.50;
 eieio-persistent slot type validation should be a bit smarter)
Date: Fri, 29 Sep 2017 20:57:20 -0400

Eric Abrahamsen <eric <at> 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.

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

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#28489: Acknowledgement (27.0.50;
 eieio-persistent slot type validation should be a bit smarter)
Date: Sat, 30 Sep 2017 11:05:44 -0700

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

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

> Eric Abrahamsen <eric <at> 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?

[0001-Fix-slot-typecheck-in-eieio-persistent.patch (text/x-diff, attachment)]

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

From: Noam Postavsky <npostavs <at> users.sourceforge.net>
To: Eric Abrahamsen <eric <at> ericabrahamsen.net>
Cc: 28489 <at> debbugs.gnu.org
Subject: Re: bug#28489: Acknowledgement (27.0.50;
 eieio-persistent slot type validation should be a bit smarter)
Date: Sat, 30 Sep 2017 17:58:54 -0400

Eric Abrahamsen <eric <at> 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))))

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

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#28489: Acknowledgement (27.0.50;
 eieio-persistent slot type validation should be a bit smarter)
Date: Sat, 30 Sep 2017 16:30:10 -0700

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

> Eric Abrahamsen <eric <at> 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

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

From: Eric Abrahamsen <eric <at> ericabrahamsen.net>
To: 28489-done <at> debbugs.gnu.org
Subject: Re: bug#28489: Acknowledgement (27.0.50;
 eieio-persistent slot type validation should be a bit smarter)
Date: Sat, 14 Oct 2017 14:13:34 +0200

Eric Abrahamsen <eric <at> ericabrahamsen.net> writes:

> On 09/28/17 20:35 PM, Noam Postavsky wrote:
>> Eric Abrahamsen <eric <at> 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.

Previous Next

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