purpleprotocol / mimalloc_rust

A Rust wrapper over Microsoft's MiMalloc memory allocator
MIT License
486 stars 42 forks source link

Allow to link dynamically against system-provided mimalloc (use pkgconf) #73

Open jirutka opened 2 years ago

jirutka commented 2 years ago

I wanted to package difftastic for Alpine Linux, but find out that it’s using bundled mimalloc library from this crate. Memory allocator is very security sensitive piece of software, using a bundled statically-linked alternative memory allocator is unacceptable for Alpine Linux, as well as for most other Linux distributions that care about security.

Can you please add support for building this crate against a system-provided mimalloc library (discovered via pkgconf) and fallback to the bundled copy of mimalloc only if not available?

thomcc commented 2 years ago

This is tricky since mimalloc isn't fully ABI stable, so updating just the allocator and leaving everything else isn't actually viable.

risicle commented 2 years ago

If you're concerned about ABI stability, would you consider an option that allowed linking with a "system" static mimalloc? This way at least the headers would match the binary version and you'd only have to worry about API stability.

Currently we (Nix) need to do Funny Tricks if we need to patch the included mimalloc to get it to e.g. build on older macos SEK versions: https://github.com/NixOS/nixpkgs/pull/174006

thomcc commented 2 years ago

That doesn't really help, unless the bindings take into account each version.

thomcc commented 2 years ago

That said, you can always use a build script override for this. It's what it's for. https://doc.rust-lang.org/cargo/reference/build-scripts.html#overriding-build-scripts

risicle commented 2 years ago

The bindings would only have to take into account API changes though, which are far less frequent and usually more intentional than ABI changes.

thomcc commented 2 years ago

They've probably slowed down now, but it's still concerning. Anyway, is there a reason a build script override doesn't work for your case? It's more or less designed for that use case.

risicle commented 2 years ago

It's actually easier and using our own tooling, but it's still more verbose and fragile compared to e.g. openssl-sys's OPENSSL_NO_VENDOR.

thomcc commented 2 years ago

Why do you consider it more fragile? I'd consider an environment variable more fragile, especially since it still runs the build script and can get it wrong (I maintain rusqlite which I believe, sadly, has this issue still in some edge cases, and haven't had the time to really dig in to figure it out).

I suppose more verbose is true, though.

risicle commented 2 years ago

Fragile from our point of view. It's fair for us to assume that a flag like OPENSSL_NO_VENDOR is supported and tested, whereas I suspect dropping in and overriding a build script potentially depends on all kinds of things like how you've set up your project.

thomcc commented 2 years ago

Sure, but supporting build script overrides greatly reduces the amount of configuations we'd need to support in the build.rs, which are exponential otherwise, and very hard to test. You're right that we don't have test coverage in this repo for build script overrides, though. It would be nice to add.