search

take-cheeze / mrusty

mruby safe bindings for Rust
Mozilla Public License 2.0
0 stars 1 forks source link

Check ownership of MrubyType in mrbgem finalizer. #2

Open take-cheeze opened 7 years ago

take-cheeze commented 7 years ago

Since it may cause double free when constructed with Mruby::new.

dragostis commented 7 years ago

Hi! I just happened to see your fork. Do you have more details about how this double free might be caused? Also, feel free to send any PRs or issues. anima-engine/mrusty is still maintained and I'm committed to improving it.

take-cheeze commented 7 years ago

@dragostis Sorry for late reply. Currently in my branch there is two mode to build mrusty.

  1. Normally as rust crate.
  2. As mrbgem(and this makes mrbgem.rake long).

So free_mrb field is added to check ownership since if MrubyType is created by mrb_open it will be owned by mrb_state. I've created the branch to create binding of html5ever (I would like to replace it with kuchiki since it has selectors). Though I haven't checked double free for 1st mode so I have this issue.

(About sending PR or issue:) Making mrusty a mrbgem may conflict with the direction of mrusty so I leaved the feature just a branch. How does mrusty developers feel of it? I know CRuby is big dependency to build mruby so I would improve stand alone minirake to replace CRuby dependency if needed.

dragostis commented 7 years ago

I think it would be great to be able to use mrusty as a mrbgem, not only mruby code from Rust. The CRuby dependency would be a no go, but this might even be solved with a cargo feature, i.e. have a mrbgem feature when compiling mrusty.

Also, let me know if you have any feedback or improvements that you would like to make to mrusty. I've been thinking about a way to have mrbgem dependencies defined directly in Cargo.toml and your minirake solution might help out with that.