update to libxml2 2.11.0 or later

flavorjones commented 1 year ago

See https://discourse.gnome.org/t/libxml2-2-11-0-released/15123 for release notes.

It doesn't look like anything in here affects nokogiri security concerns, so I'm planning to include this in a 1.15.0 release, but not a 1.14.x patch release. Opinions welcome.

Lots of yak shaving to try to make this release work ...

Summary of blockers

[x] work around windows atexit crashes: https://github.com/sparklemotion/nokogiri/pull/2871
[x] waiting for a bugfix release of libxml2 (v2.11.2) with this fix: https://gitlab.gnome.org/GNOME/libxml2/-/issues/530
[x] waiting for a bugfix release of libxml2 (v2.11.2) with this fix: https://gitlab.gnome.org/GNOME/libxml2/-/commit/d6882f6454258197916499a6546472b69baa63df
[x] waiting for upstream release with this iconv fix: https://gitlab.gnome.org/GNOME/libxml2/-/merge_requests/216

Summary of nonblocking items

[x] waiting for a ruby_memcheck release with this fix: https://github.com/Shopify/ruby_memcheck/pull/16
- https://github.com/Shopify/ruby_memcheck/releases/tag/1.3.2
[x] test upstream libxml2 on windows: https://github.com/sparklemotion/nokogiri/pull/2868
[x] test upstream libxml2 with memcheck: https://github.com/sparklemotion/nokogiri/pull/2868
[x] report bug in new upstream schema analysis (not yet v2.11): https://gitlab.gnome.org/GNOME/libxml2/-/issues/531
[x] fix compiler warnings in Nokogiri

Details

☹ ruby_memcheck is failing to parse valgrind XML files because libxml2 changed its buffering code
- 🤔 patch ruby_memcheck to use the HUGE parse option
- ☑ https://github.com/Shopify/ruby_memcheck/pull/16
- ☹ waiting for a release
  - 🤔 specify a git ref in Gemfile
  - ☹ not all CI images have git
    - 🤔 update CI docker images
    - ☑ https://github.com/sparklemotion/nokogiri/pull/2869
☹ ruby_memcheck is now flagging additional errors
- 🤔 be proactive: integration-test upstream libxml2 with ruby_memcheck to detect regressions
- https://github.com/sparklemotion/nokogiri/pull/2868
- ☑ isolate which tests are revealing the leak
- ☑ write a reproduction in C
- ☑ git-bisect libxml2
- ☑ file a bug report upstream: memory leak from `xmlSchemaValidateStream` in v2.11.x (#530) · Issues · GNOME / libxml2 · GitLab
- ☑ fixed upstream in https://gitlab.gnome.org/GNOME/libxml2/-/commit/57d88da6757457b9dfa168c1f61a5b0850883850
☹ windows segfaulting at exit
- 🤔 be proactive: integration-test upstream libxml2 on windows to detect regressions
- ☹ can't get autotools to work in the windows environments
  - ☑ add better logging to mini_portile2
  - ☑ https://github.com/flavorjones/mini_portile/pull/126
  - ☑ release mini_portile 2.8.2
- ☑ https://github.com/sparklemotion/nokogiri/pull/2868
- 🤔 seems to be a regression related to libxml2's atexit behavior, run CI with some tracing added
- ☑ make sure memory management methods are set before xmlInitParser is called
- ☑ https://github.com/sparklemotion/nokogiri/pull/2871
☹ truffleruby symbol resolution issue with xmlInitMutex (CI failure)
- 💬 some discussion at https://github.com/oracle/truffleruby/issues/3031
- 🤔 this seems to be a bug in weak symbol resolution generally
- ☑ fix submitted upstream: https://gitlab.gnome.org/GNOME/libxml2/-/merge_requests/215
- 🤔 we don't actually need the thread support detection in libxml2
- ☑ rip it out in a patch, and ensure truffleruby links libpthread when it's available
  - ☺ this is green at https://github.com/sparklemotion/nokogiri/actions/runs/4874882131 (branch flavorjones-upgrade-libxml2-2.11.0_remove-thread-detection)
- 🤔 a simpler fix might be to take the upstream fix and rip out just the libc_single_threaded bits
- ☑ write a simpler patch based on v2.11.2
☹ encoding looks broken on windows with precompiled native gems
- https://github.com/sparklemotion/nokogiri/actions/runs/4852047192/jobs/8647232273
- ☑ write a reproduction
- 🤔 how can I access a windows environment to git-bisect this?
- ☑ fire up a GCP instance and RDP into it using Remmina
- ☑ Remmina supports mounting local directories on the remote host
- ☑ bisected to 0f77167f
- 🤔 looks like an issue with the configure script
  - ☑ sent patch upstream: https://gitlab.gnome.org/GNOME/libxml2/-/merge_requests/216
☹ new behavior causing test failures was added upstream while all of the above was going on
- https://github.com/sparklemotion/nokogiri/actions/runs/4852048644/jobs/8647579888#step:8:4460
- https://gitlab.gnome.org/GNOME/libxml2/-/commit/a06eaa6119ca5b296b8105dc8c9a34ed5fc1f338
- https://gitlab.gnome.org/GNOME/libxml2/-/issues/469
- ☑ reported upstream: https://gitlab.gnome.org/GNOME/libxml2/-/issues/531
- ☑ fixed in libxml2 master
☹ compiler warnings
- ☑ remove one deprecated reference that's unnecessary
- ☑ pragma-ignore the other, since we need it to be compatible with libxml-ruby < 3.0.0

flavorjones commented 1 year ago

ruby_memcheck is flagging two new leaks in this version of libxml2. I've tracked it down to schema validation of files.

Ruby reproduction:

#! /usr/bin/env ruby

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "nokogiri", path: "."
end

ASSETS_DIR = File.expand_path(File.join(File.dirname(__FILE__), "..", "test", "files"))
PO_SCHEMA_FILE = File.join(ASSETS_DIR, "po.xsd")
PO_XML_FILE = File.join(ASSETS_DIR, "po.xml")

loop do
  xsd = Nokogiri::XML::Schema(File.read(PO_SCHEMA_FILE))

  # leaks
  xsd.validate(PO_XML_FILE)

  # does not leak
  xsd.validate(Nokogiri::XML(File.read(PO_XML_FILE)))
end

C reproduction is at https://gist.github.com/flavorjones/0902cfc1467d44417979060dd3f11971

And upstream issue was opened at memory leak from `xmlSchemaValidateStream` in v2.11.x (#530) · Issues · GNOME / libxml2 · GitLab

flavorjones commented 1 year ago

IT IS DONE. See #2866

flavorjones commented 1 year ago

sparklemotion / nokogiri