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

ParametricGeometry: Added serialization/deserialization support. #17739

Merged
merged 1 commit into from
Oct 16, 2019

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Oct 15, 2019

Fixed #17381

The assumption of this PR is that ParametricBufferGeometry.func is a self-contained function (like the ones from THREE.ParametricGeometries).

@Mugen87 Mugen87 added this to the r110 milestone Oct 16, 2019
@Mugen87 Mugen87 merged commit d16f875 into mrdoob:dev Oct 16, 2019
@mrdoob
Copy link
Owner

mrdoob commented Oct 16, 2019

Sweet! Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Oct 16, 2019

Although... Lets be aware that, if I'm not mistaken, this allows people to add evil scripts inside serialised scenes 🤔

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 24, 2019

Although... Lets be aware that, if I'm not mistaken, this allows people to add evil scripts inside serialised scenes

It's seems hard to evaluate if parametric function code is evil or not. I think I would defer this security issue to the application. If a hacker is able to manipulate a serialised scene, he can also do a lot of other funny stuff^^.

@mrdoob
Copy link
Owner

mrdoob commented Oct 24, 2019

But, imagine that I create a cool 3d model and then I add a parametric function in it that downloads access the internet and starts mining. Then I serialise it / export it to json and share it with people. Any end-user displaying that model will end up doing some bitcoin mining.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 24, 2019

I see, I see^^. Then let's revert the change since I can't think of an easy way to make the PR more safe.

@peteroupc
Copy link
Contributor

peteroupc commented Oct 26, 2019

Parametric curves and surfaces ought to be expressed in a grammar that supports only basic operations and math functions (such as sin and ln), not in a general-purpose programming language like JavaScript. A hypothetical example is the following JSON:

 { "x":"sin(u)*cos(v)", "y":"sin(u)*sin(v)", "z":"cos(v)" }

Such a grammar reduces the risks involved in arbitrary code execution because it doesn't support access to network resources, clocks, threads, etc., and neither does it support control flow statements such as loops.

However, I'm not aware of any data formats that support storing parametric curves or surfaces in a limited math grammar like this.

A while back, I wrote a script that compiles a grammar like this into an expression tree that can later be evaluated to support parametric curves and surfaces (example). This script and example are just for your information only, to show the effort involved.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 28, 2019

@peteroupc Right now, I don't think we want to add this complexity to ParametricGeometry. A workaround could be the serialization/deserialization of the pure geometry data, without parameters. The geometry could be restored in this way, however not the original parameterization (meaning the parametric function).

@mrdoob
Copy link
Owner

mrdoob commented Oct 28, 2019

@peteroupc That's interesting. 1300 LOC feels a bit too much though...

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.

Serialization of ParametricGeometry, ParametricBufferGeometry contains unserializable function
3 participants