Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow literal bitstype and symbol type parameters #203

Merged
merged 2 commits into from
Jan 7, 2015
Merged

Conversation

mbauman
Copy link
Member

@mbauman mbauman commented Jan 7, 2015

Since Julia now allows these, we should, too. Before:

julia> using HDF5, JLD
       type Foo{a}; end
       immutable Bar; x::Int; end

julia> save("test.jld", "x", Foo{:x}()); load("test.jld")
WARNING: type Foo{x} not present in workspace; reconstructing
Dict{Union(UTF8String,ASCIIString),Any} with 1 entry:
  "x" => ##Foo{x}#9613()

julia> save("test.jld", "x", Foo{1.0}()); load("test.jld")
ERROR: `is_valid_type_ex` has no method matching is_valid_type_ex(::Float64)
 in map at ./base.jl:189
 in is_valid_type_ex at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:753
 in julia_type at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:772
 in _julia_type at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:763
 in julia_type at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:17
 in jldatatype at /Users/mbauman/.julia/v0.3/HDF5/src/jld_types.jl:612
 in read at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:331
 in read at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:316
 in anonymous at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:977
 in jldopen at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:237
 in load at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:976

julia> save("test.jld", "x", Foo{Bar(1)}); load("test.jld")
WARNING: type Foo{Bar(1)} not present in workspace; reconstructing
Dict{Union(UTF8String,ASCIIString),Any} with 1 entry:
  "x" => ##Foo{Bar(1)}#9675()

After:

julia> save("test.jld", "x", Foo{:x}()); load("test.jld")
Dict{Union(UTF8String,ASCIIString),Any} with 1 entry:
  "x" => Foo{:x}()

julia> save("test.jld", "x", Foo{1.0}()); load("test.jld")
Dict{Union(UTF8String,ASCIIString),Any} with 1 entry:
  "x" => Foo{1.0}()

julia> save("test.jld", "x", Foo{Bar(1)}); load("test.jld") # Edited; this is still unsupported
WARNING: type Foo{Bar(1)} not present in workspace; reconstructing
Dict{Union(UTF8String,ASCIIString),Any} with 1 entry:
  "x" => ##Foo{Bar(1)}#9675()

Since Julia now allows these, we should, too.  Before:

```julia
julia> using HDF5, JLD
       type Foo{a}; end
       immutable Bar; x::Int; end

julia> save("test.jld", "x", Foo{:x}()); load("test.jld")
WARNING: type Foo{x} not present in workspace; reconstructing
Dict{Union(UTF8String,ASCIIString),Any} with 1 entry:
  "x" => ##Foo{x}#9613()

julia> save("test.jld", "x", Foo{1.0}()); load("test.jld")
ERROR: `is_valid_type_ex` has no method matching is_valid_type_ex(::Float64)
 in map at ./base.jl:189
 in is_valid_type_ex at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:753
 in julia_type at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:772
 in _julia_type at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:763
 in julia_type at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:17
 in jldatatype at /Users/mbauman/.julia/v0.3/HDF5/src/jld_types.jl:612
 in read at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:331
 in read at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:316
 in anonymous at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:977
 in jldopen at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:237
 in load at /Users/mbauman/.julia/v0.3/HDF5/src/JLD.jl:976

julia> save("test.jld", "x", Foo{Bar(1)}); load("test.jld")
WARNING: type Foo{Bar(1)} not present in workspace; reconstructing
Dict{Union(UTF8String,ASCIIString),Any} with 1 entry:
  "x" => ##Foo{Bar(1)}#9675()
```

After:

```julia
julia> save("test.jld", "x", Foo{:x}()); load("test.jld")
Dict{Union(UTF8String,ASCIIString),Any} with 1 entry:
  "x" => Foo{:x}()

julia> save("test.jld", "x", Foo{1.0}()); load("test.jld")
Dict{Union(UTF8String,ASCIIString),Any} with 1 entry:
  "x" => Foo{1.0}()

julia> save("test.jld", "x", Foo{Bar(1)}); load("test.jld")
Dict{Union(UTF8String,ASCIIString),Any} with 1 entry:
  "x" => Foo{Bar(1)}
```
is_valid_type_ex(e::Expr) = ((e.head == :curly || e.head == :tuple || e.head == :.) && all(map(is_valid_type_ex, e.args))) ||
(e.head == :call && (e.args[1] == :Union || e.args[1] == :TypeVar))
(e.head == :call && isa(e.args[1], Symbol))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little unfortunate that we can't be more specific here, but there's no way to determine if calling a symbol might end up as a bitstype immutable without trying it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 33d9991 on mb/bitstypes into 2237628 on master.

@simonster
Copy link
Member

Hmm. It seems like this introduces a potential security vulnerability. Using eval already gives me the creeps, but previously we had some safeguards since we only allowed Union and TypeVar calls. If we allow arbitrary call expressions, one can create a JLD file with a type like Val{rm("C:\\"; recursive=true)} and then get someone to load it. There's also no guarantee that calling print on an arbitrary bits type immutable gives you an expression that can be used to reconstruct it, since it might have its own print method or an inner constructor, and on Julia 0.3 print won't print the full path to the module in which the immutable is defined. It seems that we either need to serialize the types/type parameters using JLD or come up with our own string representation of types that can be used to reconstruct them.

As pointed out by @simonster, this ends up `eval`-ing any call written into the HDF5 file as a type parameter and is a security threat.
@mbauman
Copy link
Member Author

mbauman commented Jan 7, 2015

Yeah, agreed. It gave me the heebie-jeebies, too, which is why I left that inline note. The rest of this is simple enough (and likely covers the vast majority of use-cases) that I say we merge this and punt the non-literal immutable problem to another issue.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 5c7db4d on mb/bitstypes into 2237628 on master.

@timholy
Copy link
Member

timholy commented Jan 7, 2015

I don't have any concerns to add (good catch on the eval risks), and I see you've re-restricted it. I'll let the two of you decide if/when to merge (AppVeyor failure seems to be a network problem, nothing to do with your code).

@mbauman mbauman changed the title Allow bitstype and symbol type parameters Allow literal bitstype and symbol type parameters Jan 7, 2015
mbauman added a commit that referenced this pull request Jan 7, 2015
Allow literal bitstype and symbol type parameters
@mbauman mbauman merged commit 65f5dd9 into master Jan 7, 2015
@mbauman mbauman deleted the mb/bitstypes branch January 7, 2015 20:30
@mbauman
Copy link
Member Author

mbauman commented Jan 7, 2015

I think this is a simple step in the right direction, so I hit the button. I opened issue #204 to address the custom immutable problem.

@simonster
Copy link
Member

Yes, I agree that this is a strict improvement. For the moment, we might want to throw an error when saving types parametrized by bits types that we can't read.

@timholy
Copy link
Member

timholy commented Jan 8, 2015

+1 for the error upon save.

@tkelman
Copy link
Contributor

tkelman commented Jan 21, 2015

Every version of HDF5 released since this was merged needs to update its minimum supported julia version in REQUIRE. This broke HDF5 and MAT on a colleague's machine who was still using 0.3.2.

@mbauman
Copy link
Member Author

mbauman commented Jan 22, 2015

Yikes, I'm sorry about that, Tony. I'll be sure to think about things like this in the future. Thanks for taking care of the REQUIRE changes.

@tkelman
Copy link
Contributor

tkelman commented Jan 22, 2015

It was just the one version, right? That's what it looked like in metadata. It would be a bit more convenient for searching and navigation if recent tags of the package were showing up in the github repo here.

@timholy
Copy link
Member

timholy commented Jan 22, 2015

Ouch. Sorry about that. Many thanks for discovering and fixing the problem.

@timholy
Copy link
Member

timholy commented Jan 22, 2015

Are tags not showing up in checkouts? I have most of them, but I wonder if bypassing Pkg.publish() fails to send out the tags to the rest of the world?

@simonster
Copy link
Member

If you don't use Pkg.publish(), I believe you have to run git push --tags to push the tags to package repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants