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 to link dynamically against system-provided mimalloc (use pkgconf) #73

Open
jirutka opened this issue Nov 13, 2021 · 10 comments
Open

Comments

@jirutka
Copy link

jirutka commented Nov 13, 2021

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
Copy link
Contributor

thomcc commented Nov 13, 2021

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

@risicle
Copy link

risicle commented May 22, 2022

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: NixOS/nixpkgs#174006

@thomcc
Copy link
Contributor

thomcc commented May 22, 2022

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

@thomcc
Copy link
Contributor

thomcc commented May 22, 2022

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
Copy link

risicle commented May 22, 2022

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
Copy link
Contributor

thomcc commented May 22, 2022

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
Copy link

risicle commented May 22, 2022

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
Copy link
Contributor

thomcc commented May 22, 2022

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
Copy link

risicle commented May 22, 2022

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
Copy link
Contributor

thomcc commented May 22, 2022

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.

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

No branches or pull requests

3 participants