[0.x] Fix memory leaks of string buffers

[jhawthorn]

In a few places we were passing malloc'd memory to rb_str_new (and variants) and returning the resulting VALUE without freeing the passed C string buffer.

One of these leaks was introduced by #171 (we can see that some calls to free were lost in the rewrite), but others seem to predate that.

After this change I'm able to run ASAN_OPTIONS="detect_leaks=1" RUBY_FREE_AT_EXIT=1 bundle exec rake with an ASAN-built Ruby without errors.

require "commonmarker"
10.times do
  100.times do
    CommonMarker.render_html("hello, world\n"*10000)
  end
  puts `ps -orss= -p #$$`
end

❯ bundle exec ruby test.rb
74736
112112
112112
128496
172016
203120
203120
203120
241392
266480

cc @anticomputer @phillmv

[kivikakk]

LGTM :) Thank you!

[gjtorikian]

After two years of managing this fork I think it’s fair to ask: can y’all please move to the 1.x/Rust-backed version?

[gjtorikian]

@jhawthorn Please, don’t interpret my comment as hostility. I literally had a panic attack after the PR merged because I wasn’t sure if the C-backed gem was still auto deploying or not; it’s been a while since the last cut and I didn’t know if I would get a ping about the gem not being released.

I would like to remove the part of my brain that worries about this old gem being a part of your infrastructure. Let me know if there’s anything I can do to help update.

[phillmv]

I literally had a panic attack after the PR merged because I wasn’t sure if the C-backed gem was still auto deploying or not

Aw! S'all good, we were monitoring it on our end, and I would've done something had it failed. Sorry to give you feels about it.

can y’all please move to the 1.x/Rust-backed version?

We'd rather like to do so! However, John has Specific and Nuanced Views on the Safe Way to interact way Ruby from Rust, and they are incompatible with the magnus library due to, iirc, concerns around exception handling and memory management. I believe John's preference for talking to Rust is to use the FFI gem to interface with .so. (not to speak on John's behalf; as a non-systems programmer, i can't accurately reproduce his concerns).

Since this is all unfunded, volunteer work, it's just not something we've had time to audit/propose to fix.

I would like to remove the part of my brain that worries about this old gem being a part of your infrastructure.

Durr, here's what - we'll fork this repo and get this dependency your hands. It won't happen right away, but I hereby relieve you of the burden of worrying about publishing this gem.

[gjtorikian]

we'll fork this repo and get this dependency your hands. It won't happen right away, but I hereby relieve you of the burden of worrying about publishing this gem.

Fabulous! I will delete the branch as soon as I am able. Y'all might also be interested in https://github.com/ioquatix/markly, which is a cut-off/fork from right before this moved to Rust.

believe John's preference for talking to Rust is to use the FFI gem to interface with .so. (not to speak on John's behalf; as a non-systems programmer, i can't accurately reproduce his concerns).

Yeah, this is something I actually did before relying entirely on Magnus: write a little FFI .h and .c, and expose a C API in Rust. I'd be super curious to know what the issues are.

I know various Shopify employees are also building out OSS Rust + Ruby tooling, so there's at least an opportunity for collaboration here. But you do you! Thanks for freeing us. ☺️

[phillmv]

I will delete the branch as soon as I am able.

Gimme a few weeks to push this around on my todo list before deleting anything 😅 🙇‍♀️. I'll let you know when it's totally off your hands!