Setting -maxsigcachesize to zero causes segfault #9759

1. 
   jnewbery commented at 3:29 PM on February 14, 2017: member

   The signature cache is set up here:

   // To be called once in AppInit2/TestingSetup to initialize the signatureCache
   void InitSignatureCache()
   {
       size_t nMaxCacheSize = GetArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE) * ((size_t) 1 << 20);
       if (nMaxCacheSize <= 0) return;
       size_t nElems = signatureCache.setup_bytes(nMaxCacheSize);
       LogPrintf("Using %zu MiB out of %zu requested for signature cache, able to store %zu elements\n",
               (nElems*sizeof(uint256)) >>20, nMaxCacheSize>>20, nElems);
   }
   

   if -maxsigcachesize is set to zero, then we just return from the function back into AppInit2. Note that we don't call signatureCache.setup_bytes().

   We later try to use the sigcache and segfault.

   Here's a comment from the CuckcooCache::cache class:

       /** You must always construct a cache with some elements via a subsequent
        * call to setup or setup_bytes, otherwise operations may segfault.
        */
   

   and indeed we do:

   (gdb) bt
   [#0](/bitcoin-bitcoin/0/)  __memcmp_sse4_1 () at ../sysdeps/x86_64/multiarch/memcmp-sse4.S:1525
   [#1](/bitcoin-bitcoin/1/)  0x000056317c16411b in base_blob<256u>::Compare (other=..., this=0x0) at ./uint256.h:45
   [#2](/bitcoin-bitcoin/2/)  operator== (b=..., a=...) at ./uint256.h:47
   [#3](/bitcoin-bitcoin/3/)  CuckooCache::cache<uint256, (anonymous namespace)::SignatureCacheHasher>::contains (this=0x56317c767100 <(anonymous namespace)::signatureCache+32>, erase=254, e=...) at ./cuckoocache.h:447
   [#4](/bitcoin-bitcoin/4/)  (anonymous namespace)::CSignatureCache::Get (this=0x56317c7670e0 <(anonymous namespace)::signatureCache>, erase=254, entry=...) at script/sigcache.cpp:70
   [#5](/bitcoin-bitcoin/5/)  CachingTransactionSignatureChecker::VerifySignature (this=0x7f1d077fae00, vchSig=std::vector of length 71, capacity 72 = {...}, pubkey=..., sighash=...) at script/sigcache.cpp:107
   [#6](/bitcoin-bitcoin/6/)  0x000056317c34e4a2 in TransactionSignatureChecker::CheckSig (this=0x7f1d077fae00, vchSigIn=std::vector of length 72, capacity 72 = {...}, vchPubKey=..., scriptCode=..., sigversion=SIGVERSION_BASE) at script/interpreter.cpp:1268
   [#7](/bitcoin-bitcoin/7/)  0x000056317c347143 in EvalScript (stack=std::vector of length 2, capacity 2 = {...}, script=..., flags=flags@entry=65503, checker=..., sigversion=sigversion@entry=SIGVERSION_BASE, serror=serror@entry=0x7f1d077faf3c) at script/interpreter.cpp:902
   [#8](/bitcoin-bitcoin/8/)  0x000056317c34f2cb in VerifyScript (scriptSig=..., scriptPubKey=..., witness=0x7f1cd4000ff8, flags=65503, checker=..., serror=serror@entry=0x7f1d077faf3c) at script/interpreter.cpp:1429
   [#9](/bitcoin-bitcoin/9/)  0x000056317c1a1653 in CScriptCheck::operator() (this=this@entry=0x7f1d077faf00) at validation.cpp:1359
   [#10](/bitcoin-bitcoin/10/) 0x000056317c1a6073 in CheckInputs (tx=..., state=..., inputs=..., fScriptChecks=fScriptChecks@entry=true, flags=flags@entry=65503, cacheStore=cacheStore@entry=true, txdata=..., pvChecks=0x0) at validation.cpp:1448
   [#11](/bitcoin-bitcoin/11/) 0x000056317c1b8186 in AcceptToMemoryPoolWorker (pool=..., state=..., ptx=std::shared_ptr (count 3, weak 0) 0x7f1cd4000bf0, fLimitFree=fLimitFree@entry=true, pfMissingInputs=pfMissingInputs@entry=0x0, nAcceptTime=<optimized out>, plTxnReplaced=0x0,
       fOverrideMempoolLimit=false, nAbsurdFee=@0x7f1d077fc508: 10000000, vHashTxnToUncache=std::vector of length 0, capacity 0) at validation.cpp:953
   [#12](/bitcoin-bitcoin/12/) 0x000056317c1b9a1b in AcceptToMemoryPoolWithTime (pool=..., state=..., tx=std::shared_ptr (count 3, weak 0) 0x7f1cd4000bf0, fLimitFree=fLimitFree@entry=true, pfMissingInputs=pfMissingInputs@entry=0x0, nAcceptTime=<optimized out>, plTxnReplaced=0x0,
       fOverrideMempoolLimit=false, nAbsurdFee=10000000) at validation.cpp:1021
   [#13](/bitcoin-bitcoin/13/) 0x000056317c1b9b89 in AcceptToMemoryPool (pool=..., state=..., tx=std::shared_ptr (count 3, weak 0) 0x7f1cd4000bf0, fLimitFree=fLimitFree@entry=true, pfMissingInputs=pfMissingInputs@entry=0x0, plTxnReplaced=plTxnReplaced@entry=0x0, fOverrideMempoolLimit=false,
       nAbsurdFee=10000000) at validation.cpp:1036
   [#14](/bitcoin-bitcoin/14/) 0x000056317c2ef51a in CMerkleTx::AcceptToMemoryPool (state=..., nAbsurdFee=<optimized out>, this=0x7f1d077fd0a0) at wallet/wallet.cpp:3950
   [#15](/bitcoin-bitcoin/15/) CWallet::CommitTransaction (this=0x56317cc8b740, wtxNew=..., reservekey=..., connman=0x56317caee7e0, state=...) at wallet/wallet.cpp:2751
   [#16](/bitcoin-bitcoin/16/) 0x000056317c2a706e in SendMoney (address=..., nValue=nValue@entry=100000000, fSubtractFeeFromAmount=fSubtractFeeFromAmount@entry=false, wtxNew=...) at wallet/rpcwallet.cpp:372
   [#17](/bitcoin-bitcoin/17/) 0x000056317c2b2871 in sendtoaddress (request=...) at wallet/rpcwallet.cpp:431
   [#18](/bitcoin-bitcoin/18/) 0x000056317c15a33b in CRPCTable::execute (this=<optimized out>, request=...) at rpc/server.cpp:492
   [#19](/bitcoin-bitcoin/19/) 0x000056317c2184b7 in HTTPReq_JSONRPC (req=0x7f1ce4001620) at httprpc.cpp:193
   [#20](/bitcoin-bitcoin/20/) 0x000056317c21c002 in std::function<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)>::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const (
       __args#1="", __args#0=0x7f1ce4001620, this=0x7f1ce40016b0) at /usr/include/c++/5/functional:2267
   [#21](/bitcoin-bitcoin/21/) HTTPWorkItem::operator() (this=0x7f1ce4001680) at httpserver.cpp:51
   [#22](/bitcoin-bitcoin/22/) WorkQueue<HTTPClosure>::Run (this=0x56317ca410d0) at httpserver.cpp:132
   [#23](/bitcoin-bitcoin/23/) HTTPWorkQueueRun (queue=0x56317ca410d0) at httpserver.cpp:361
   [#24](/bitcoin-bitcoin/24/) 0x00007f1d0eabdc80 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
   [#25](/bitcoin-bitcoin/25/) 0x00007f1d0e2d06ba in start_thread (arg=0x7f1d077fe700) at pthread_create.c:333
   [#26](/bitcoin-bitcoin/26/) 0x00007f1d0e00682d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
   

   Trivially reproducible for me.

   I believe the fix is to add a minimum signature cache size constant MIN_SIG_CACHE_SIZE and change this:

       if (nMaxCacheSize <= 0) return;
   

   to this:

       if (nMaxCacheSize <= MIN_SIG_CACHE_SIZE) {
           nMaxCacheSize = MIN_SIG_CACHE_SIZE
           LogPrintf("maxsigcachesize must be at least %d. Setting maxsigcachesize to %d", MIN_SIG_CACHE_SIZE, MIN_SIG_CACHE_SIZE);
           InitWarning(strprintf(_(maxsigcachesize must be at least %d. Setting maxsigcachesize to %d", MIN_SIG_CACHE_SIZE, MIN_SIG_CACHE_SIZE));
   }
   

   in InitSignatureCache()

2. 
   jnewbery commented at 3:30 PM on February 14, 2017: member

   @JeremyRubin

3. fanquake added the label Bug on Feb 14, 2017
4. 
   TheBlueMatt commented at 12:44 AM on February 15, 2017: member

   0.14 tag, probably, because its a regression (and hopefully easy fix).

5. fanquake added this to the milestone 0.14.0 on Feb 15, 2017
6. 
   ghost commented at 4:32 AM on February 15, 2017: none

   I guess You mean if (nMaxCacheSize < MIN_SIG_CACHE_SIZE) ?

7. 
   sipa commented at 9:15 AM on February 15, 2017: member

   @jnewbery Seems like a simple fix. Create a PR?

8. 
   JeremyRubin commented at 10:25 AM on February 15, 2017: contributor

   Seems fine! I think you could also just call setup_bytes(max(0, nMaxCacheSize)) and it should work OK (if I recall correctly, a call to setup_bytes with argument 0 will construct a cache with only 2 elements).

9. 
   JeremyRubin commented at 10:30 AM on February 15, 2017: contributor

   Woud also be ok to call setup(0) inside cuckoocache constructor, if you want to not have that requirement.

   On a meta note, it's a little concerning that we check the cache when nMaxCacheSize is 0. If this is a common case, it may be worthwhile to otherwise disable trying to check the cache when it is 0.

10. 
    laanwj commented at 3:58 PM on February 15, 2017: member

    Are you going to PR the (trivial) fix or should I?

    I don't think this should hold up 0.14. Easiest workaround is to just not set -maxsigcachesize to zero.

11. 
    jnewbery commented at 5:26 PM on February 15, 2017: member

    @laanwj I'll PR today

12. 
    jnewbery commented at 7:27 PM on February 15, 2017: member

    I've confirmed that @JeremyRubin is correct and that setup_bytes(0) creates a cache with two elements.

    nMaxCacheSize is unsigned, so can never be < 0. The fix is therefore just to remove the line:

    if (nMaxCacheSize <= 0) return;
    

    (and add a comment). PR is here: https://github.com/bitcoin/bitcoin/pull/9770/files

    I've tested this with -maxsigcachesize set to zero and bitcoind no longer segfaults.

13. laanwj closed this on Feb 16, 2017

    ---

14. DrahtBot locked this on Sep 8, 2021

Milestone
0.14.0