Bug 82246 (atexit)

Summary: Libraries should not call atexit()
Product: HarfBuzz Reporter: Thomas Klausner <tk>
Component: srcAssignee: Behdad Esfahbod <freedesktop>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium CC: freedesktop
Version: unspecified   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Patch addressing the issue.

Description Thomas Klausner 2014-08-06 12:03:53 UTC
Created attachment 104147 [details]
Patch addressing the issue.

Shared objects, particularly when they are dynamically loaded, should
not call atexit(). At process exit, rtld may unload them before
calling atexit handlers, at which point the code -- e.g., free_langs
-- is no longer in the process's address space. atexit thus should
never be used in a library: it should be limited to applications.

On the other hand, a shared object's destructors are called before the
shared object is unloaded. Using __attribute__((__destructor__)) marks
such a function.

The attached patch addresses this issue, and removes a superfluous
inline.
Comment 1 Behdad Esfahbod 2014-08-06 14:44:14 UTC
Thanks.  Did you actually notice this in any real setting?  There are more invocations of atexit, I'll switch them to use destructors if they are supported.
Comment 2 Thomas Klausner 2014-08-06 14:47:56 UTC
I'm currently debugging why gmpc dumps core during exiting, and this is one of the atexit() calls I've found that I thought might be the cause. The patch wasn't sufficient though, so I'm currently looking for more...
Comment 3 Behdad Esfahbod 2014-08-06 16:14:31 UTC
Quoting man atexit:

   """Linux Notes
       Since glibc 2.2.3, atexit() (and on_exit(3)) can be used within a shared library to establish functions that are called when the shared library is unloaded."""


So, I think this is as safe as we care on glibc-based systems.

From:

  http://msdn.microsoft.com/en-ca/library/tze57ck3.aspx

   """The code in the atexit function should not contain any dependency on any DLL which could have already been unloaded when the atexit function is called."""

Which makes me believe that it's safe to use it in DLLs as long as it doesn't call other DLLs.  However, there's also _onexit:

  http://msdn.microsoft.com/en-ca/library/zk17ww08.aspx

that specifically says:

   """In the case when _onexit is called from within a DLL, routines registered with _onexit run on a DLL's unloading after DllMain is called with DLL_PROCESS_DETACH.

_onexit is a Microsoft extension. For ANSI portability, use atexit."""

Reading around the net:

  http://arstechnica.com/civis/viewtopic.php?f=20&t=438235

I get the impression that atexit() is also called during DLL unload just like _onexit().

It used to be an issue on bionic / Android, but seems to have been fixed in Nov 2012:

  https://code.google.com/p/android/issues/detail?id=6455

As such, I think this is working as intended.

Thanks for the report though.  Until someone comes with a legitimate system that this doesn't work, I'll leave it as is.  Only then I'll add conditionals for all the systems that we know this is safe to do.
Comment 5 Behdad Esfahbod 2014-08-06 16:21:36 UTC
Ok, thanks.  I'll try a whitelist approach then.
Comment 6 Behdad Esfahbod 2014-08-06 17:36:39 UTC
commit 38fb30d7420a4b01f99cee31baa8c3990a1d1c5f
Author: Behdad Esfahbod <behdad@behdad.org>
Date:   Wed Aug 6 13:34:49 2014 -0400

    Use atexit() only if it's safe to call from shared library
    
    Apparently they are not (advertised as?) safe on BSD systems.
    We ignore the case of static libraries.
    
    Whitelisted on glibc, Android, and MSVC / mingw.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=82246

diff --git a/src/hb-private.hh b/src/hb-private.hh
index 5a4ca69..1b39e57 100644
--- a/src/hb-private.hh
+++ b/src/hb-private.hh
@@ -130,6 +130,31 @@
 #  define STRICT
 #endif
 
+#if HAVE_ATEXIT
+/* atexit() is only safe to be called from shared libraries on certain
+ * platforms.  Whitelist.
+ * https://bugs.freedesktop.org/show_bug.cgi?id=82246 */
+#  if defined(__linux) && defined(__GLIBC_PREREQ)
+#    if __GLIBC_PREREQ(2,3)
+/* From atexit() manpage, it's safe with glibc 2.2.3 on Linux. */
+#      define HB_USE_ATEXIT 1
+#    endif
+#  elif defined(_MSC_VER) || defined(__MINGW32__)
+/* For MSVC:
+ * http://msdn.microsoft.com/en-ca/library/tze57ck3.aspx
+ * http://msdn.microsoft.com/en-ca/library/zk17ww08.aspx
+ * mingw32 headers say atexit is safe to use in shared libraries.
+ */
+#    define HB_USE_ATEXIT 1
+#  elif defined(__ANDROID__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6))
+/* This was fixed in Android NKD r8 or r8b:
+ * https://code.google.com/p/android/issues/detail?id=6455
+ * which introduced GCC 4.6:
+ * https://developer.android.com/tools/sdk/ndk/index.html
+ */
+#    define HB_USE_ATEXIT 1
+#  endif
+#endif
Comment 7 Thomas Klausner 2014-08-07 17:47:34 UTC
Thank you, that is an improvement.
How about the leak on the platforms that don't have atexit support for unloading shared objects? Would you consider a variant of my patch for them, or at least for another set of whitelisted platforms?
Comment 8 Behdad Esfahbod 2014-08-07 18:12:40 UTC
Ok, I see what you mean.

Some background as to why I introduced atexit:

Inititally, I was using static C++ classes with constructors and destructors for this purpose.  However, when we were porting Chrome to use HarfBuzz-ng, the Chrome team didn't like it.  They are strictly against static constructors since those add directly to the app start time.

One alternative was to move them into function scope.  That delays the initialization to the first time the function is entered.  However, that would require linking libstdc++, which is something we want to avoid.

As such, I turned them into plain old static C data and implemented thread-safe initialization explicitly.  At that point (ie. avoiding C++), I found atexit() the easiest way to reclaim the memory portably.

Now, perhaps it's ok to use destructor attributes and they might be more portable, or perhaps we need both...  In that case might be easier to introduce an hb_onexit() polyfill.

I'll checkout simpleops to decide:

  http://cgit.freedesktop.org/~ranma42/simpleops/log/

Feel free to submit patch though, I'm a bit busy with other work right now.  Thanks!
Comment 9 Thomas Klausner 2014-08-08 22:51:36 UTC
Thanks for your comments.
I'm told by people whose expertise I trust that the destructor markup is very portable, but I can't confirm this myself.

Anyway, more to do here, leave this open for now.
Comment 10 Behdad Esfahbod 2014-08-10 19:48:22 UTC
Thanks.  "Very portable" is heartwarming but doesn't change the fact that it still needs whitelisting as it doesn't work with the same syntax on Windows.

Lets add a hb_atexit() that either simply is an alias for atexit() or to a function that fills in a static array that is then freed by an attribute/pragma'ed function.
Comment 11 Behdad Esfahbod 2018-08-23 21:52:36 UTC
Marking fixed as there's no actionable item left.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.