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

Investigate xmlCleanupParser segfault with libxml2 master #2059

Closed
flavorjones opened this issue Aug 4, 2020 · 4 comments
Closed

Investigate xmlCleanupParser segfault with libxml2 master #2059

flavorjones opened this issue Aug 4, 2020 · 4 comments

Comments

@flavorjones
Copy link
Member

@flavorjones flavorjones commented Aug 4, 2020

Describe the bug

When using libxml2 containing this commit:

https://gitlab.gnome.org/GNOME/libxml2/-/commit/9fa3200cb366c726f7c8ef234282603bb9e8816d

Nokogiri will segfault:

==487730== Invalid read of size 8
==487730==    at 0x493A0E6: ruby_sized_xfree (gc.c:10145)
==487730==    by 0x493A0E6: ruby_xfree (gc.c:10152)
==487730==    by 0xB2A412D: xmlCleanupCharEncodingHandlers (in /home/flavorjones/tmp/libxml2/lib/libxml2.so.2.9.10)
==487730==    by 0xB2ACCB8: xmlCleanupParser__internal_alias.part.0 (in /home/flavorjones/tmp/libxml2/lib/libxml2.so.2.9.10)
==487730==    by 0x4011F5A: _dl_fini (dl-fini.c:138)
==487730==    by 0x4E7FA26: __run_exit_handlers (exit.c:108)
==487730==    by 0x4E7FBDF: exit (exit.c:139)
==487730==    by 0x4E5D0B9: (below main) (libc-start.c:342)
==487730==  Address 0x480 is not stack'd, malloc'd or (recently) free'd
==487730== 
[BUG] Segmentation fault at 0x0000000000000480

To Reproduce

(1) Build libxml2 shared libraries:

PREFIX="${HOME}/tmp/libxml2"
INSTALL_DIR=${PREFIX}/lib
./configure --prefix="${PREFIX}" --without-python --without-readline --with-c14n --with-debug --with-threads --with-iconv=yes --host=x86_64-pc-linux-gnu CFLAGS="-O2"
make install

(obviously your host platform may be different)

(2) Set up to use those header files and shared library:

export LD_LIBRARY_PATH=${INSTALL_DIR}
export CFLAGS=-I${PREFIX}/include/libxml2
export LDFLAGS="-lxml2 -L${INSTALL_DIR}"

(3) Build Nokogiri against these libraries

bundle exec rake clean
NOKOGIRI_USE_SYSTEM_LIBRARIES=t bundle exec rake compile

(4) Run any ruby script that uses Nokogiri:

$ valgrind ruby -Ilib -rnokogiri -e 'Nokogiri::HTML("<div>hi</div>")'
==490818== Memcheck, a memory error detector
==490818== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==490818== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==490818== Command: ruby -Ilib -rnokogiri -e Nokogiri::HTML("\<div\>hi\</div\>")
==490818== 
==490818== Warning: client switching stacks?  SP change: 0x1ffeffea30 --> 0x1ffe8020e0
==490818==          to suppress, use: --max-stackframe=8374608 or greater
==490818== Invalid write of size 1
==490818==    at 0x4A95455: reserve_stack (thread_pthread.c:820)
==490818==    by 0x4A99B3F: ruby_init_stack (thread_pthread.c:846)
==490818==    by 0x108950: main (main.c:48)
==490818==  Address 0x1ffe8020e0 is on thread 1's stack
==490818==  in frame #0, created by reserve_stack (thread_pthread.c:775)
==490818== 
==490818== Warning: client switching stacks?  SP change: 0x1ffe8020e0 --> 0x1ffeffeb50
==490818==          to suppress, use: --max-stackframe=8374896 or greater
==490818== Invalid read of size 8
==490818==    at 0x493A0E6: ruby_sized_xfree (gc.c:10145)
==490818==    by 0x493A0E6: ruby_xfree (gc.c:10152)
==490818==    by 0xB2A412D: xmlCleanupCharEncodingHandlers (in /home/flavorjones/tmp/libxml2/lib/libxml2.so.2.9.10)
==490818==    by 0xB2ACCB8: xmlCleanupParser__internal_alias.part.0 (in /home/flavorjones/tmp/libxml2/lib/libxml2.so.2.9.10)
==490818==    by 0x4011F5A: _dl_fini (dl-fini.c:138)
==490818==    by 0x4E7FA26: __run_exit_handlers (exit.c:108)
==490818==    by 0x4E7FBDF: exit (exit.c:139)
==490818==    by 0x4E5D0B9: (below main) (libc-start.c:342)
==490818==  Address 0x480 is not stack'd, malloc'd or (recently) free'd
==490818== 
[BUG] Segmentation fault at 0x0000000000000480

Expected behavior

I would expect this to not segfault.

Additional context

I git-bisected the problem to this commit:

https://gitlab.gnome.org/GNOME/libxml2/-/commit/9fa3200cb366c726f7c8ef234282603bb9e8816d

which is related to this issue:

https://gitlab.gnome.org/GNOME/libxml2/-/issues/153

and related discussion is at:

@nwellnhof
Copy link

@nwellnhof nwellnhof commented Aug 4, 2020

(libxml2 maintainer here) It seems that Nokogiri tells libxml2 to use its own memory management handlers which then call into Ruby code. Apparently, this doesn't work in an ELF destructor, probably because Ruby has already cleaned up its internal state.

Maybe it's possible that Nokogiri calls xmlCleanupParser when unloading the module or at some other point before the ELF destructor is invoked. This should make the second call in the destructor a no-op.

Another solution on the libxml2 side is to skip the call in the destructor if custom memory management functions are installed. This seems like a good idea in any case.

gnomesysadmins pushed a commit to GNOME/libxml2 that referenced this issue Aug 4, 2020
Calling a custom deallocation function in the global destructor could
cause all kinds of unexpected problems. See for example

    sparklemotion/nokogiri#2059

Only clean up if memory is managed with malloc/free.
@nwellnhof
Copy link

@nwellnhof nwellnhof commented Aug 4, 2020

The check in libxml2 mentioned above is a good idea in any case. So this should be fixed in libxml2 now: https://gitlab.gnome.org/GNOME/libxml2/-/commit/956534e02ef280795a187c16f6ac04e107f23c5d

Thanks for the report!

@flavorjones
Copy link
Member Author

@flavorjones flavorjones commented Aug 4, 2020

@nwellnhof Thanks for the quick and thorough response. I've confirmed that https://gitlab.gnome.org/GNOME/libxml2/-/commit/956534e02ef280795a187c16f6ac04e107f23c5d does indeed address this problem and Nokogiri no longer segfaults.

I really appreciate it!

@flavorjones flavorjones closed this Aug 4, 2020
@nwellnhof
Copy link

@nwellnhof nwellnhof commented Aug 4, 2020

@flavorjones I'm always grateful if people test pre-release versions of libxml2. It's extremely valuable to find such issues as early as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants