Fork me on GitHub
#clojure-dev
<
2018-08-02
>
bronsa14:08:05

@alexmiller what do we want to do about that deserialization test?

bronsa14:08:37

to me it looks completely useless -- what's the point of a test that checks for deserialisation throwing when we already checked that serialisation is not possible?

bronsa14:08:25

I have no problem regenerating the seralised binaries but I don't see much point in maintaining that test

Alex Miller (Clojure team)14:08:48

well the idea was to manually construct the object that an attacker could produce that previously was able to run arbitrary code

Alex Miller (Clojure team)14:08:01

to verify it doesn’t (still) work

Alex Miller (Clojure team)14:08:11

I have been on the fence about maintaining it

bronsa14:08:49

right but isn't it just testing that if an attacker produces code that is unserialisable then we can't deserialise it? I may be reading the test wrong

bronsa15:08:34

but it seems to me that the test is true by construction

bronsa15:08:46

I don't remember the history behind it tho

Alex Miller (Clojure team)15:08:58

don’t all tests verify something you think is true?

Alex Miller (Clojure team)15:08:38

the object is hard-coded and generated from a modified version of the code before the check was introduced iirc

Alex Miller (Clojure team)15:08:34

I’d be ok with just commenting it out - I’m not sure that it’s worth the cost of maintenance

Alex Miller (Clojure team)15:08:06

I do think it’s a good canary for your change though that the class name has changed - does this mean that new clojure wouldn’t find proxy class files from old clojure?

bronsa15:08:30

I can regenerate the classfiles this evening once I get to a machine with java 8 through 11, or I can comment it, I leave it up to you :)

bronsa15:08:41

it just means it won't reuse the cached class

bronsa15:08:48

and will generate a fresh one

Alex Miller (Clojure team)15:08:15

isn’t that foiling aot then?

bronsa15:08:24

which should not be an issue as you interop with proxy objects type hinting the interfaces

bronsa15:08:56

no, AOT compiled proxy instances will still refer to the AOT compiled classes

bronsa15:08:20

it's just that if you have an AOT compiled proxy pre-patch + a JIT compiled proxy post-patch, they won't reuse the same underlying impl

bronsa15:08:30

but -- will do more testing in a few hours to confirm

Alex Miller (Clojure team)15:08:44

stepping back steps further, you might ask whether redefing interfaces is a supported use case at all

bronsa15:08:49

it might break code if somebody was crazy enough to type hint a method call using {:tag (class the-proxy-object)} -- but there's nothing special about it, it's the same situation that would happen for defrecord/detype

bronsa15:08:18

well we support reloading of definterfaces for deftype/defrecord

bronsa15:08:30

so I don't see why proxy should be more special there

Alex Miller (Clojure team)15:08:50

just want to make sure we’re not adding scope

Alex Miller (Clojure team)15:08:20

seems like redefinition of protocols kind of has the opposite problem - too tied to particular instances rather than names

bronsa15:08:26

I think either way the current behaviour is broken: definterface supports realoading but reloading of proxy backed by a reloaded definterface breaks

bronsa15:08:59

we could go the other way and make definterface not reloadable

bronsa15:08:09

but I have a hunch that that would definitely break code :)

Alex Miller (Clojure team)15:08:20

well I’d rather make everything more reloadable :)

bronsa15:08:43

(notably this is one of the first reloading bugs where AOT is not involved :) )

Alex Miller (Clojure team)15:08:17

inames is a sorted set so should always hash the same

Alex Miller (Clojure team)15:08:01

interfaces is arbitrarily ordered

bronsa15:08:15

good point

Alex Miller (Clojure team)15:08:53

not sure if it matters given that you’re going to a more fine-grained approach anyways

Alex Miller (Clojure team)15:08:36

is that concat doing anything?

bronsa15:08:13

no -- I've been doing too much ocaml recently

Alex Miller (Clojure team)15:08:20

wouldn’t just (hash (conj interfaces super)) be sufficient?

bronsa15:08:31

an alternative patch I was considering wouldn't change the naming scheme but instead would decide whether to use the cached class or regenerate one

bronsa15:08:46

we probably want (hash (sort (conj interfaces super)))

Alex Miller (Clojure team)15:08:12

are the interface objects here comparable?

bronsa15:08:56

(hash (sort (map hash (conj interfaces super))))

bronsa15:08:34

>seems like redefinition of protocols kind of has the opposite problem - too tied to particular instances rather than names I didn't get this btw

bronsa15:08:35

in my mental model being tied to instances rather than to names is what we want (speaking about interfaces anyway)

bronsa15:08:57

and all the problems we've had till now has been about correctly pointing the names at the latest instances

Alex Miller (Clojure team)15:08:28

the problem you run into with protocol redefinition is that records extending those protocol are tied to the old protocol interface class

Alex Miller (Clojure team)15:08:45

that is, they are class instance based

bronsa15:08:29

so what you're saying is what clojure needs is a meta-object protocol, am I hearing it right? :P