RFC: Migrate serialization format to JSON

[ararslan]

This is probably hot garbage at the moment, but I figured I'd put it up to get feedback while I continue to improve it.

Also note that the dependency on JSON.jl is likely temporary, until I can implement a mini JSON parser in this package.

Fixes #15

[jrevels]

Great to finally get rid of the JLD dependency!

[jrevels]

Might want to break out this duplicated assert into its own function (either rewriting the message to be the same for load/save or just pass a flag giving the context).

[jrevels]

"produced by" --> "originally written by" would be clearer IMO

[jrevels]

Could we change this to SUPPORTED_TYPES?

[jrevels]

Please consult the manual before adding a test like this.

[KristofferC]

!!!!!!!!

[ararslan]

You're right Jarrett, my apologies. We're no strangers to unit testing; you know the rules and so do I.

[jrevels]

I'm sure you'd do this eventually anyway, but just to be explicit, we should definitely delete this commented out stuff before merging...what a joy it will be to be rid of it!

[ararslan]

Yeah definitely

[ararslan]

I'm not sure what to make of that error message. I'm not seeing it locally. :/

Edit: I would have been, I had been checking on 0.6.

[ararslan]

TESTS ARE PASSING ON 0.6

[ararslan]

I've verified locally that the new serialization tests pass on 0.7. There are other issues that are unrelated to this PR, likely due to the 0.7 macro hygiene chaos, which are keeping the full test suite from passing.

[KristofferC]

Isn't this a bit scary to call on something that comes from a json file? Should there be a whitelist of types and just switch on that?

[ararslan]

Yeah I suppose so. The hope is that people will only read in JSON files that were produced by this package, but I suppose it's pretty easy to falsify that if you know the expected format.

[ararslan]

Apparently Jarrett is unfazed by this.

[ararslan]

I tried adding this to address @KristofferC's comment but for some reason it fails with an UndefVarError for NefariousCode rather than the expected failure...

diff --git a/src/serialization.jl b/src/serialization.jl
index 52715ed..ba3e9e1 100644
--- a/src/serialization.jl
+++ b/src/serialization.jl
@@ -22,6 +22,10 @@ function recover(x::Vector)
     length(x) == 2 || throw(ArgumentError("Expecting a vector of length 2"))
     typename = x[1]::String
     fields = x[2]::Dict
+    if !any(t->ismatch(Regex(string('^', t, "({[^}]+)?\$")), typename), SUPPORTED_TYPES)
+        throw(ArgumentError("Cannot deserialize type '$typename'; only BenchmarkTools " *
+                            "types can be deserialized"))
+    end
     T = eval(parse(typename))::Type
     fc = fieldcount(T)
     xs = Vector{Any}(fc)
diff --git a/test/SerializationTests.jl b/test/SerializationTests.jl
index 7a51ae7..54f1f8f 100644
--- a/test/SerializationTests.jl
+++ b/test/SerializationTests.jl
@@ -78,6 +78,22 @@ end
         end
     end

+    withtempdir() do
+        tmp = joinpath(pwd(), "tmp.json")
+        open(tmp, "w") do f
+            print(f, """
+            [{"Julia":"1.2.3","BenchmarkTools":"3.2.1"},[["NefariousCode",{}]]]
+            """)
+        end
+        try
+            BenchmarkTools.load(tmp)
+            error("aaahhh")
+        catch err
+            @test err isa ArgumentError
+            @test contains(err.msg, "Cannot deserialize type 'NefariousCode'")
+        end
+    end
+
     @test_throws ArgumentError BenchmarkTools.recover([1])
 end

[KristofferC]

I think you are missing an end brace in your regex, so should be "({[^}]+})?\$"). However, you could just use a type like BenchmarkTools.Benchmark{println(1+1)}".

On the other hand, I think jld files can already execute arbitrary code so I this shouldn't be any worse security than before.

[jrevels]

LGTM! Some of these removed lines were a long time coming.