Skip to content

Commit

Permalink
Backport Ruby upstream patch to fix seg faults in libxml2/Nokogiri
Browse files Browse the repository at this point in the history
As sparklemotion/nokogiri#2785 (comment)
explains, there is a bug in the Ruby interpreter
(https://bugs.ruby-lang.org/issues/19580) that has been fixed upstream
(ruby/ruby#7663) that causes a seg fault
during shutdown with libxml2/Nokogiri.

We patched the Ruby interpreter in CI to work around the problem
(https://gitlab.com/gitlab-org/gitlab-build-images/-/merge_requests/672)
in https://gitlab.com/gitlab-org/gitlab/-/issues/390313, but it
appears the seg faults have appeared in production now. On GitLab.com,
this week we have seen more than 20 cases with the error:

```
[BUG] Segmentation fault at 0x0000000000000440
```

We could also work around this problem by setting
`NOKOGIRI_LIBXML_MEMORY_MANAGEMENT=default`, but this may cause
unexpected memory growth since Ruby no longer manages the memory (see
https://github.com/sparklemotion/nokogiri/pull/2843/files).

Let's just fix the interpreter since we'd also need to make sure that
environment variable is set in any environment that uses Nokogiri
(including Rake tasks).

Changelog: fixed
  • Loading branch information
stanhu committed Jun 1, 2023
1 parent b6060a7 commit bd949e2
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 10 deletions.
23 changes: 23 additions & 0 deletions config/patches/ruby/fix-ruby-xfree-for-libxml2-2.7.patch
@@ -0,0 +1,23 @@
diff --git a/gc.c b/gc.c
index 67a709ff79..98785901f4 100644
--- a/gc.c
+++ b/gc.c
@@ -10174,8 +10174,16 @@ ruby_xrealloc2_body(void *ptr, size_t n, size_t size)
void
ruby_sized_xfree(void *x, size_t size)
{
- if (x) {
- objspace_xfree(&rb_objspace, x, size);
+ if (LIKELY(x)) {
+ /* It's possible for a C extension's pthread destructor function set by pthread_key_create
+ * to be called after ruby_vm_destruct and attempt to free memory. Fall back to mimfree in
+ * that case. */
+ if (LIKELY(GET_VM())) {
+ objspace_xfree(&rb_objspace, x, size);
+ }
+ else {
+ ruby_mimfree(x);
+ }
}
}

23 changes: 23 additions & 0 deletions config/patches/ruby/fix-ruby-xfree-for-libxml2-3.0.patch
@@ -0,0 +1,23 @@
diff --git a/gc.c b/gc.c
index 5d0c342206..2bfff21004 100644
--- a/gc.c
+++ b/gc.c
@@ -10905,8 +10905,16 @@ ruby_xrealloc2_body(void *ptr, size_t n, size_t size)
void
ruby_sized_xfree(void *x, size_t size)
{
- if (x) {
- objspace_xfree(&rb_objspace, x, size);
+ if (LIKELY(x)) {
+ /* It's possible for a C extension's pthread destructor function set by pthread_key_create
+ * to be called after ruby_vm_destruct and attempt to free memory. Fall back to mimfree in
+ * that case. */
+ if (LIKELY(GET_VM())) {
+ objspace_xfree(&rb_objspace, x, size);
+ }
+ else {
+ ruby_mimfree(x);
+ }
}
}

23 changes: 23 additions & 0 deletions config/patches/ruby/fix-ruby-xfree-for-libxml2-3.1.patch
@@ -0,0 +1,23 @@
diff --git a/gc.c b/gc.c
index 030a4627bd..1c96f6401f 100644
--- a/gc.c
+++ b/gc.c
@@ -11763,8 +11763,16 @@ ruby_xrealloc2_body(void *ptr, size_t n, size_t size)
void
ruby_sized_xfree(void *x, size_t size)
{
- if (x) {
- objspace_xfree(&rb_objspace, x, size);
+ if (LIKELY(x)) {
+ /* It's possible for a C extension's pthread destructor function set by pthread_key_create
+ * to be called after ruby_vm_destruct and attempt to free memory. Fall back to mimfree in
+ * that case. */
+ if (LIKELY(GET_VM())) {
+ objspace_xfree(&rb_objspace, x, size);
+ }
+ else {
+ ruby_mimfree(x);
+ }
}
}

23 changes: 23 additions & 0 deletions config/patches/ruby/fix-ruby-xfree-for-libxml2-3.2.patch
@@ -0,0 +1,23 @@
diff --git a/gc.c b/gc.c
index 54400e4f6b..96d43ac8ac 100644
--- a/gc.c
+++ b/gc.c
@@ -12614,8 +12614,16 @@ ruby_xrealloc2_body(void *ptr, size_t n, size_t size)
void
ruby_sized_xfree(void *x, size_t size)
{
- if (x) {
- objspace_xfree(&rb_objspace, x, size);
+ if (LIKELY(x)) {
+ /* It's possible for a C extension's pthread destructor function set by pthread_key_create
+ * to be called after ruby_vm_destruct and attempt to free memory. Fall back to mimfree in
+ * that case. */
+ if (LIKELY(GET_VM())) {
+ objspace_xfree(&rb_objspace, x, size);
+ }
+ else {
+ ruby_mimfree(x);
+ }
}
}

20 changes: 10 additions & 10 deletions config/software/ruby.rb
Expand Up @@ -104,16 +104,16 @@
# be fixed.
end

# Enable custom patch created by ayufan that allows to count memory allocations
# per-thread. This is asked to be upstreamed as part of https://github.com/ruby/ruby/pull/3978
if version.start_with?('2.7')
patch source: 'thread-memory-allocations-2.7.patch', plevel: 1, env: env
elsif version.start_with?('3.0')
patch source: 'thread-memory-allocations-3.0.patch', plevel: 1, env: env
elsif version.start_with?('3.1')
patch source: 'thread-memory-allocations-3.1.patch', plevel: 1, env: env
elsif version.start_with?('3.2')
patch source: 'thread-memory-allocations-3.2.patch', plevel: 1, env: env
# Two patches:
# 1. Enable custom patch created by ayufan that allows to count memory allocations
# per-thread. This is asked to be upstreamed as part of https://github.com/ruby/ruby/pull/3978
# 2. Backport Ruby upstream patch to fix seg faults in libxml2/Nokogiri: https://bugs.ruby-lang.org/issues/19580
# This has been merged for Ruby 3.3 but not yet backported: https://github.com/ruby/ruby/pull/7663
patches = %w[thread-memory-allocations fix-ruby-xfree-for-libxml2]
ruby_version = Gem::Version.new(version).canonical_segments[0..1].join('.')

patches.each do |patch_name|
patch source: "#{patch_name}-#{ruby_version}.patch", plevel: 1, env: env
end

# copy_file_range() has been disabled on recent RedHat kernels:
Expand Down

0 comments on commit bd949e2

Please sign in to comment.