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 memfd_create to be used with older glibc (<2.27) #2146

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LizardWizzard
Copy link

@LizardWizzard LizardWizzard commented Oct 2, 2023

Hi! Thank you for your great work!

What does this PR do

While upgrading to more recent version of nix I've faced an issue on a system that has older glibc.

Since this commit nix started to refer to memfd_create symbol instead of using a syscall. Glibc started to export this symbol since 2.27 and lowest version supported by rust project is now 2.17, so it would be nice to keep compatibility on a similar level if possible. See comments in code.

Found that there was similar issue in libsystemd-rs: lucab/libsystemd-rs#144

Thanks in advance!

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

@LizardWizzard LizardWizzard force-pushed the fix-memfd-create-for-older-libc branch from 4f4cffa to 656ea9b Compare October 2, 2023 09:52
@asomers
Copy link
Member

asomers commented Oct 2, 2023

Are you trying to create a dylib target? If so, your trouble should be fixed by #2049 .

@LizardWizzard
Copy link
Author

Thanks for quick response! No, I’m building a binary target

@asomers
Copy link
Member

asomers commented Oct 2, 2023

Thanks for quick response! No, I’m building a binary target

Do you mean an executable? Ordinary Rust executables should not have a problem with this, as I understand, unless you're using a very old compiler or something. Is there anything unusual in your setup, and have you tested with the lastest Nix release?

@LizardWizzard
Copy link
Author

Sorry for long response

Do you mean an executable?

Yes

Is there anything unusual in your setup, and have you tested with the lastest Nix release?

The only unusual thing here is old glibc. I've prepared a reproducer that gives the same error: https://github.com/LizardWizzard/nix_memfd_create_repro it uses latest nix version available on crates io (0.27.1) and latest stable release of rustc (1.72.1)

My exact conditions are a bit different, but this is a small reproducer that shows the same problem. See code that triggers the problem here: https://github.com/LizardWizzard/nix_memfd_create_repro/blob/master/pure_nix_repro/src/main.rs

docker image I use is a default centos:7 with some installed packages.

To run the example in this repo only call to docker build . is needed. There will be an error during build phase. Relevant part of it is: undefined reference to memfd_create

Thank you!

@LizardWizzard LizardWizzard force-pushed the fix-memfd-create-for-older-libc branch from 656ea9b to 685d0cb Compare October 6, 2023 11:32
@LizardWizzard
Copy link
Author

I rebased the patch on top of current master to resolve conflict in changelog. All pipelines that failed have this error message: Failed to start an instance: FAILED_PRECONDITION: Monthly free compute limit exceeded!

daniestevez added a commit to daniestevez/systemd-journal-logger.rs that referenced this pull request Oct 18, 2023
The memfd_create() function is available in glib >= 2.27, but Rust
supports glibc >= 2.17. In order to keep compatibility with old
glibc versions, this performs a memfd_create syscall directly. This
syscall is available in the Linux kernel >= 3.17.

There is currently a pull request
nix-rust/nix#2146
in the nix crate that will use the syscall with all the glibc versions,
in order to account for older glibc versions, while calling the libc
function in other libc's such as musl where the function has existed
from the beginning. When this pull request is merged, this change
can be undone and memfd_create() from nix can be called instead.

Signed-off-by: Daniel Estévez <daniel@destevez.net>
@LizardWizzard
Copy link
Author

Hey @SteveLauC! Sorry for the ping, will you have some time to take a look at the PR? Thanks in advance!

@SteveLauC
Copy link
Member

Hey @SteveLauC! Sorry for the ping

No worry about the ping, feel free to do so when needed:)


Are you trying to create a dylib target? If so, your trouble should be fixed by #2049

Do you mean an executable? Ordinary Rust executables should not have a problem with this, as I understand, unless you're using a very old compiler or something. Is there anything unusual in your setup, and have you tested with the lastest Nix release?

Different from the problem #2049 tries to solve, the author actually uses the memfd_create() symbol on a system with glibc 2.17

To address this issue, we should either:

  1. Merge this PR, use the raw syscall
  2. Done some runtime check, use the libc binding if available, fall back to syscall if not

For solution 2, this is actually what std and rustix do when they encounter a problem like this

@LizardWizzard
Copy link
Author

Yeah, I briefly looked at weak stuff. Apparently it has a bigger scope compared to reverting to raw syscalls, because I havent seen weak being already used anywhere in nix. If there are no objections option 1 looks ok to me. Otherwise probably weak needs to be introduced separately and then used here and maybe in other API's

@LizardWizzard
Copy link
Author

@SteveLauC what do you think if we merge this one? Do you have any objections?

@SteveLauC
Copy link
Member

I do tend to use the libc wrapper as syscalls are inherently unsafe, so I prefer approach 2

cc @asomers, thoughts on this?

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.

3 participants