#267 Static initialization order fiasco

[Dhyanesh Damania]

There is static initialization order fiasco while creating GlobalCacheBase. The global_cache_lock static variable may not be initialized when GlobalCacheBase is constructed.

static StackPtr<Mutex> global_cache_lock(new Mutex);

One of the instances where this is possible is from typo_edit_dist.cpp

static GlobalCache<TypoEditDistanceInfo> typo_edit_dist_info_cache("keyboard");

This was found by AddressSanitizer:

https://code.google.com/p/address-sanitizer/

[Dhyanesh Damania]

Here is a proposed patch.

Edit: I needed to add a missing include in one unrelated file as well (for ptrdiff_t).

Index: common/cache.cpp
===================================================================
RCS file: /sources/aspell/aspell/common/cache.cpp,v
retrieving revision 1.10
diff -u -r1.10 cache.cpp
--- common/cache.cpp    7 Jul 2011 06:21:06 -0000       1.10
+++ common/cache.cpp    27 May 2013 09:55:13 -0000
@@ -5,8 +5,11 @@

 namespace aspell {

-static StackPtr<Mutex> global_cache_lock(new Mutex);
 static GlobalCacheBase * first_cache = 0;
+static StackPtr<Mutex> & global_cache_lock() {
+  static StackPtr<Mutex> instance(new Mutex);
+  return instance;
+}

 void Cacheable::copy() const
 {
@@ -70,7 +73,7 @@
 GlobalCacheBase::GlobalCacheBase(const char * n)
   : name (n)
 {
-  LOCK(global_cache_lock);
+  LOCK(global_cache_lock());
   next = first_cache;
   prev = &first_cache;
   if (first_cache) first_cache->prev = &next;
@@ -80,14 +83,14 @@
 GlobalCacheBase::~GlobalCacheBase()
 {
   detach_all();
-  LOCK(global_cache_lock);
+  LOCK(global_cache_lock());
   *prev = next;
   if (next) next->prev = prev;
 }

 bool reset_cache(const char * which)
 {
-  LOCK(global_cache_lock);
+  LOCK(global_cache_lock());
   bool any = false;
   for (GlobalCacheBase * i = first_cache; i; i = i->next)
   {
Index: common/hash.hpp
===================================================================
RCS file: /sources/aspell/aspell/common/hash.hpp,v
retrieving revision 1.10
diff -u -r1.10 hash.hpp
--- common/hash.hpp     7 Jul 2011 06:21:07 -0000       1.10
+++ common/hash.hpp     27 May 2013 09:55:13 -0000
@@ -12,6 +12,7 @@
 #ifndef autil__hash_hh
 #define autil__hash_hh

+#include <cstddef>
 #include <utility>
 #include <functional>

[Dhyanesh Damania]

There are more issues that I found. Here is a complete patch.

Index: common/cache.cpp
===================================================================
RCS file: /sources/aspell/aspell/common/cache.cpp,v
retrieving revision 1.10
diff -u -r1.10 cache.cpp
--- common/cache.cpp    7 Jul 2011 06:21:06 -0000   1.10
+++ common/cache.cpp    5 Jun 2013 06:31:15 -0000
@@ -5,8 +5,11 @@

 namespace aspell {

-static StackPtr<Mutex> global_cache_lock(new Mutex);
 static GlobalCacheBase * first_cache = 0;
+static StackPtr<Mutex> & global_cache_lock() {
+  static StackPtr<Mutex> instance(new Mutex);
+  return instance;
+}

 void Cacheable::copy() const
 {
@@ -70,7 +73,7 @@
 GlobalCacheBase::GlobalCacheBase(const char * n)
   : name (n)
 {
-  LOCK(global_cache_lock);
+  LOCK(global_cache_lock());
   next = first_cache;
   prev = &first_cache;
   if (first_cache) first_cache->prev = &next;
@@ -80,14 +83,14 @@
 GlobalCacheBase::~GlobalCacheBase()
 {
   detach_all();
-  LOCK(global_cache_lock);
+  LOCK(global_cache_lock());
   *prev = next;
   if (next) next->prev = prev;
 }

 bool reset_cache(const char * which)
 {
-  LOCK(global_cache_lock);
+  LOCK(global_cache_lock());
   bool any = false;
   for (GlobalCacheBase * i = first_cache; i; i = i->next)
   {
Index: common/convert.cpp
===================================================================
RCS file: /sources/aspell/aspell/common/convert.cpp,v
retrieving revision 1.59
diff -u -r1.59 convert.cpp
--- common/convert.cpp  7 Jul 2011 07:59:03 -0000   1.59
+++ common/convert.cpp  5 Jun 2013 06:31:16 -0000
@@ -862,9 +862,19 @@
   // Cache
   //

-  static GlobalCache<Decode> decode_cache("decode");
-  static GlobalCache<Encode> encode_cache("encode");
-  static GlobalCache<NormTables> norm_tables_cache("norm_tables");
+  static GlobalCache<Decode> &decode_cache() {
+    static GlobalCache<Decode> instance("decode");
+    return instance;
+  }
+  static GlobalCache<Encode> &encode_cache() {
+    static GlobalCache<Encode> instance("encode");
+    return instance;
+  }
+  static GlobalCache<NormTables> &norm_tables_cache() {
+    static GlobalCache<NormTables> instance("norm_tables");
+    return instance;
+  }
+

   //////////////////////////////////////////////////////////////////////
   //
@@ -1189,10 +1199,10 @@

   Convert::InitRet Convert::init(const Config & c, ParmStr in, ParmStr out)
   {
-    INIT_RET_ON_ERR(decode, setup(decode_c, &decode_cache, &c, in));
+    INIT_RET_ON_ERR(decode, setup(decode_c, &decode_cache(), &c, in));
     decode_ = decode_c.get();
     in_code_ = decode_->key;
-    INIT_RET_ON_ERR(encode, setup(encode_c, &encode_cache, &c, out));
+    INIT_RET_ON_ERR(encode, setup(encode_c, &encode_cache(), &c, out));
     encode_ = encode_c.get();
     out_code_ = encode_->key;

@@ -1216,9 +1226,9 @@

   Convert::InitRet Convert::init_norm_from(const Config & c, ParmStr in, ParmStr out)
   {
-    INIT_RET_ON_ERR(encode, setup(norm_tables_, &norm_tables_cache, &c, out));
+    INIT_RET_ON_ERR(encode, setup(norm_tables_, &norm_tables_cache(), &c, out));

-    INIT_RET_ON_ERR(decode, setup(decode_c, &decode_cache, &c, in));
+    INIT_RET_ON_ERR(decode, setup(decode_c, &decode_cache(), &c, in));
     decode_ = decode_c.get();
     in_code_ = decode_->key;

@@ -1243,9 +1253,9 @@
   Convert::InitRet Convert::init_norm_to(const Config & c, ParmStr in, ParmStr out,
                                       ParmStr norm_form)
   {
-    INIT_RET_ON_ERR(decode, setup(norm_tables_, &norm_tables_cache, &c, in));
+    INIT_RET_ON_ERR(decode, setup(norm_tables_, &norm_tables_cache(), &c, in));

-    INIT_RET_ON_ERR(encode, setup(encode_c, &encode_cache, &c, out));
+    INIT_RET_ON_ERR(encode, setup(encode_c, &encode_cache(), &c, out));
     encode_ = encode_c.get();
     out_code_ = encode_->key;

Index: common/hash.hpp
===================================================================
RCS file: /sources/aspell/aspell/common/hash.hpp,v
retrieving revision 1.10
diff -u -r1.10 hash.hpp
--- common/hash.hpp 7 Jul 2011 06:21:07 -0000   1.10
+++ common/hash.hpp 5 Jun 2013 06:31:16 -0000
@@ -12,6 +12,7 @@
 #ifndef autil__hash_hh
 #define autil__hash_hh

+#include <cstddef>
 #include <utility>
 #include <functional>

Index: lib/new_fmode.cpp
===================================================================
RCS file: /sources/aspell/aspell/lib/new_fmode.cpp,v
retrieving revision 1.24
diff -u -r1.24 new_fmode.cpp
--- lib/new_fmode.cpp   7 Jul 2011 07:59:04 -0000   1.24
+++ lib/new_fmode.cpp   5 Jun 2013 06:31:16 -0000
@@ -483,8 +483,6 @@
     return no_err;
   }

-  static GlobalCache<FilterModeList> filter_modes_cache("filter_modes");
-
   PosibErr<void> set_mode_from_extension (Config * config, ParmString filename, FILE * in) 
   {
     RET_ON_ERR_SET(static_cast<ModeNotifierImpl *>(config->filter_mode_notifier)
@@ -504,6 +502,7 @@

   PosibErr<FilterModeList *>  ModeNotifierImpl::get_filter_modes()
   {
+    static GlobalCache<FilterModeList> filter_modes_cache("filter_modes");
     if (!filter_modes_) {
       //FIXME is filter-path proper for filter mode files ???
       //      if filter-options-path better ???
Index: modules/speller/default/lang_impl.cpp
===================================================================
RCS file: /sources/aspell/aspell/modules/speller/default/lang_impl.cpp,v
retrieving revision 1.5
diff -u -r1.5 lang_impl.cpp
--- modules/speller/default/lang_impl.cpp   7 Jul 2011 07:59:04 -0000   1.5
+++ modules/speller/default/lang_impl.cpp   5 Jun 2013 06:31:16 -0000
@@ -78,8 +78,6 @@
     , {"norm-form",           KeyInfoString, "nfc", "", 0, FOR_CONFIG}
   };

-  static GlobalCache<LangImpl> language_cache("language");
-
   PosibErr<void> LangImpl::setup(const String & lang, const Config * config)
   {
     //
@@ -588,6 +586,7 @@

   PosibErr<LangImpl *> new_lang_impl(const Config & config, ParmStr lang)
   {
+    static GlobalCache<LangImpl> language_cache("language");
     if (!lang)
       return get_cache_data(&language_cache, &config, config.retrieve("lang"));
     else
Index: modules/speller/default/typo_editdist.cpp
===================================================================
RCS file: /sources/aspell/aspell/modules/speller/default/typo_editdist.cpp,v
retrieving revision 1.11
diff -u -r1.11 typo_editdist.cpp
--- modules/speller/default/typo_editdist.cpp   7 Jul 2011 07:59:04 -0000   1.11
+++ modules/speller/default/typo_editdist.cpp   5 Jun 2013 06:31:16 -0000
@@ -76,11 +76,10 @@
     return e(word_size-1,target_size-1);
   }

-  static GlobalCache<TypoEditDistanceInfo> typo_edit_dist_info_cache("keyboard");
-
   PosibErr<void> setup(CachePtr<const TypoEditDistanceInfo> & res,
                        const Config * c, const LangImpl * l, ParmString kb)
   {
+    static GlobalCache<TypoEditDistanceInfo> typo_edit_dist_info_cache("keyboard");
     PosibErr<TypoEditDistanceInfo *> pe = get_cache_data(&typo_edit_dist_info_cache, c, l, kb);
     if (pe.has_err()) return pe;
     res.reset(pe.data);

[Kevin Atkinson]

This issue has moved to GitHub: https://github.com/GNUAspell/aspell/issues/512