#267 Static initialization order fiasco

0.61
open
nobody
None
5
2013-06-05
2013-05-27
Dhyanesh Damania
No

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/

Discussion

  • 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>
    
     
    Last edit: Dhyanesh Damania 2013-05-27
  • 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);
    
     
    Attachments