Bug 285870 - Bug in atexit code in rare specific case + patch to correct it
Summary: Bug in atexit code in rare specific case + patch to correct it
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 14.2-RELEASE
Hardware: amd64 Any
: --- Affects Some People
Assignee: Kyle Evans
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-04-03 19:44 UTC by Aurélien Croc de Suray
Modified: 2025-04-05 00:49 UTC (History)
5 users (show)

See Also:
kevans: mfc-stable14?
kevans: mfc-stable13?


Attachments
Code example which generates the bug (571 bytes, text/plain)
2025-04-03 19:44 UTC, Aurélien Croc de Suray
no flags Details
lib/libc/stdlib/atexit.c patch to correct the bug (2.90 KB, patch)
2025-04-03 19:48 UTC, Aurélien Croc de Suray
no flags Details | Diff
git(1) diff against libc (6.94 KB, patch)
2025-04-04 14:41 UTC, Kyle Evans
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aurélien Croc de Suray 2025-04-03 19:44:19 UTC
Created attachment 259304 [details]
Code example which generates the bug

Since at least five years the use of dlib-cpp as a shared library loaded with dlopen and freed with dlclose lead the main program to crash with a segmentation fault when it exit (example: when the "pdlib" PHP wrapper is used php crashes after the main routine returns).

After debugging the problem I noticed that it crashes because __cxa_atexit was called during a call to a function (previously registered with a __cxa_atexit call) during __cxa_finalize when the shared library was closed.

I wrote an simple code which mimics the bug. You will find it attached to this bug report. To use it:
 $ c++ -o atexit atexit.cpp
 $ c++ -fPIC -shared -o libatexit.so atexit.cpp -DSHARED
 $ ./atexit
CA instance destroyed
closed
[1]    28802 segmentation fault (core dumped)  ./atexit

lldb confirms that it crashes because it tries to call a function which doesn't exists anymore:

(lldb) bt
* thread #1, name = 'atexit', stop reason = signal SIGSEGV: address not mapped to object (fault address: 0x824784b90)
  * frame #0: 0x0000000824784b90
    frame #1: 0x000000082275af7f libc.so.7`__cxa_finalize + 319
    frame #2: 0x000000082275b4dc libc.so.7`exit + 76
    frame #3: 0x000000082267bc41 libc.so.7`__libc_start1 + 305
    frame #4: 0x0000000000201780 atexit`_start at crt1_s.S:83
Comment 1 Aurélien Croc de Suray 2025-04-03 19:48:49 UTC
Created attachment 259305 [details]
lib/libc/stdlib/atexit.c patch to correct the bug
Comment 2 Aurélien Croc de Suray 2025-04-03 19:49:26 UTC
I wrote a libc patch which set a flag in order to let __cxa_finalize redoing its job if a new callback was added for the current DSO during the __cxa_finalize call. It solve the problem.

See atexit.patch
Comment 3 Kyle Evans freebsd_committer freebsd_triage 2025-04-03 20:03:45 UTC
CC kib and jhb as we had chatted about this a little bit on IRC.  I'll volunteer for ATF'ifying the reproducer, but don't have a strong opinion on fixing it (this approach vs. checking __atexit/__atexit->ind having changed before reprocessing)
Comment 4 Kyle Evans freebsd_committer freebsd_triage 2025-04-04 14:41:11 UTC
Created attachment 259319 [details]
git(1) diff against libc

As promised, a set of ATF tests for __cxa_finalize handling.  This only tests basic functionality and the bug in question, not any specific ordering.
Comment 5 commit-hook freebsd_committer freebsd_triage 2025-04-05 00:49:07 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=ee9ce1078c596f5719f312feedd616ab0fb41dc9

commit ee9ce1078c596f5719f312feedd616ab0fb41dc9
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2025-04-05 00:47:54 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2025-04-05 00:47:54 +0000

    libc: tests: add some tests for __cxa_atexit handling

    This adds a basic test that __cxa_atexit works, and also adds some tests
    for __cxa_atexit handlers registered in the middle of __cxa_finalize.

    PR:             285870

 lib/libc/tests/stdlib/Makefile                     |   2 +
 lib/libc/tests/stdlib/cxa_atexit_test.c (new)      | 132 +++++++++++++++++++++
 lib/libc/tests/stdlib/libatexit/Makefile (new)     |  11 ++
 lib/libc/tests/stdlib/libatexit/libatexit.cc (new) |  67 +++++++++++
 4 files changed, 212 insertions(+)
Comment 6 commit-hook freebsd_committer freebsd_triage 2025-04-05 00:49:09 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=23427c8e1fedb9fc68ad0bd27a59c7ffd2b3008c

commit 23427c8e1fedb9fc68ad0bd27a59c7ffd2b3008c
Author:     Aurélien Croc de Suray <freebsd@ap2c.com>
AuthorDate: 2025-04-05 00:47:53 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2025-04-05 00:47:53 +0000

    libc: allow __cxa_atexit handlers to be added during __cxa_finalize

    science/dlib-cpp reveals an interesting scenario that works fine on
    other platforms but not on FreeBSD; notably, it ends up creating a new
    global object from some destructor which is called during
    __cxa_finalize.  This breaks when libdlib is dlopen()ed and then
    subsequently dlclose()ed, as we never end up invoking the created
    object's dtor until program exit when the shlib is already unmapped.

    Fix it by noting when we're in the middle of __cxa_finalize for a dso,
    and then restarting the search if __cxa_atexit() was called in the
    middle somewhere.

    We wait until we've processed the initial set before starting over and
    processing the newly added handlers as if it were a complete set of
    handlers added during runtime.  The alternative is calling them as
    they're added to maintain a LIFO in terms of total ordering, but in
    theory a constructor could add another global object that also needs to
    be destroyed, and that object needs to be destroyed after the one that
    constructed it to avoid creating unexpected lifetime issues.

    This manifests in the pdlib PHP extension for dlib crashing, see [0].

    [0] https://github.com/goodspb/pdlib/issues/39

    PR:             285870
    Reviewed by:    kevans (also supplied commit message)
    MFC after:      1 week

 lib/libc/stdlib/atexit.c | 61 ++++++++++++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 25 deletions(-)