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

Update to libxml 2.12.0 or later #3031

Closed
flavorjones opened this issue Nov 18, 2023 · 3 comments
Closed

Update to libxml 2.12.0 or later #3031

flavorjones opened this issue Nov 18, 2023 · 3 comments

Comments

@flavorjones
Copy link
Member

flavorjones commented Nov 18, 2023

Release notes are at https://gitlab.gnome.org/GNOME/libxml2/-/releases/v2.12.0

I don't see any security-related changes, so I'm planning this for a Nokogiri v1.16.0 relesae that will coincide with Ruby 3.3 support.

I'm seeing some challenges with a naive upgrade, so I'm creating this issue to track the conversation. I'll keep this description up to date.


the situation

libxml 2.12 makes some changes related to thread-local storage, primarily in this upstream commit. These changes are similar in nature to the changes in 2.11 made that update challenging (see #2865 for context), having to do with weak references to pthread symbols combined with the fact that libpthread was merged into glibc in glibc v2.34.

the naive first attempt

Simply updating libxml2 will result in errors while building libxml2, where the linker tries to resolve pthread symbols while linking xmllint.

resolving pthread

Trying to explicitly resolve libpthread:

But we find another problem when installing precompiled native gems on musl, which is that nokogiri.so references ld-linux-x86_64.so.2 as a "needed" dependency.

Is this problem related? Probably, but it's not clear what's going on.

I could continue this approach by finishing the work to support musl in rake-compiler dock started at rake-compiler/rake-compiler-dock#75, but

  1. I'm not sure that's the right thing to do
  2. that approach couples two significant changes together, each of which has their own risk factors, which feels doubly risky.

upstream fix?

Is this a problem with upstream libxml2? I actually think it is ...

I've submitted upstream fixes:

Applying those patches locally:

But now we're once again still pulling in libwinpthread on windows and ld-linux on linux ... there's something else going on here.

thread local storage

Looking at the compiled object files in libxml2.a, I can see that __tls_get_addr is a new "unresolved" dependency which, on glibc systems in ld-linux.so (which doesn't exist on musl) and presumably on windows does something similar to cause libwinpthread. I'm doing some reading about thread local storage here:

https://maskray.me/blog/2021-02-14-all-about-thread-local-storage

The symbol is being referenced because we're using -fPIC, and it seems like this is a reference that cannot be made weak and is libc-dependent (meaning it differs between musl and glibc).

Configuring libxml2 --without-tls avoids this issue, which is now what #3032 does.

@flavorjones
Copy link
Member Author

updated description to reflect the upstream bugfix MR that I submitted for pthread symbol support in glibc < 2.34

@flavorjones
Copy link
Member Author

updated description to include upstream patch partial success, and more info on the thread local storage features added to libxml2

flavorjones added a commit that referenced this issue Nov 18, 2023
**What problem is this PR intended to solve?**

#3031

This approach uses the patches submitted upstream at:

- https://gitlab.gnome.org/GNOME/libxml2/-/merge_requests/229
- https://gitlab.gnome.org/GNOME/libxml2/-/merge_requests/230

and turns off libxml2's thread-local storage feature, which is an issue
when precompiling for musl on glibc systems. If and when we can build a
separate musl native gem, we can revisit this decision.
@flavorjones
Copy link
Member Author

Closed by #3032

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 a pull request may close this issue.

1 participant