Handle locked pages more robustly (Fixes issue #1462) #1699

1. 
   laanwj commented at 9:50 AM on August 22, 2012: member

   I've investigated issue #1462 and in my experiments, the memory allocation pattern with a many transient objects almost guarantees that after the initialization of bitcoin only one of the pages is actually locked. This means that most of the keying material can end up in swap.

   I considered two ways to address this:

   1. A special memory pool (using boost::pool) of locked pages
   2. Keep track of which pages are locked with a lock count

   I first tried (1), but it ended up non-optimal. boost::pool is very wasteful with tiny objects (in this case, char), as the minimum chunk size seems to be the pointer size, resulting in a blow-up of 8× on 64 bit architectures. As locked pages are a limited resource, this should be avoided.

   So I went with the second alternative. This solution is also easier to test than a full-blown custom memory pool. I've added an autotester.

2. 
   BitcoinPullTester commented at 11:21 AM on August 22, 2012: none

   Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/c96ca68f9a39f8040cce6b4139d7a84a8604414b for binaries and test log.

   This could happen for one of several reasons:

   1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
   2. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
   3. The test suite fails on either Linux i386 or Win32
3. 
   laanwj commented at 11:27 AM on August 22, 2012: member

   Hmm this is interesting, CCrypter calls calls mlock / munlock directly. It's good that we have Windows. Will update these to use the LockedPageManager as well.

4. 
   sipa commented at 11:27 AM on August 22, 2012: member

   Nice, this was needed.

   You need to fix the mingw build issue, though.

5. 
   laanwj commented at 11:42 AM on August 22, 2012: member

   Should be solved now.

6. 
   gavinandresen commented at 2:02 PM on August 22, 2012: contributor

   ACK from my Mac.

7. 
   gmaxwell commented at 2:20 PM on August 22, 2012: contributor

   Should we do something useful with mlock failure? At least log something?

   I wonder how much of a lost cause this is— as the encryption key gets handled by json or the GUI...

8. 
   laanwj commented at 2:28 PM on August 22, 2012: member

   Re: mlock failure, maybe log a warning once, to prevent logspam.

   I don't agree this is a lost cause. Even though it's not water tight yet in the case of wallet encryption, it is a move in the right direction.

   It does protect the wallet private keys in the case someone has the wallet on an encrypted volume or an USB stick, but hasn't encrypted swap...

9. 
   gmaxwell commented at 3:03 PM on August 22, 2012: contributor

   Log once is better than not at all. Logspam would be bad too. Perhaps just log stats on shutdown?

10. 
    sipa commented at 8:55 PM on August 22, 2012: member

    ACK as soon as BitcoinBuildTester confirms the mingw build is fixed.

11. 
    BitcoinPullTester commented at 2:21 AM on August 23, 2012: none

    Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/d2be69cf9182c8a0ca12cb8c7baf2dc213c120d3 for binaries and test log.

    This could happen for one of several reasons:

    1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
    2. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
    3. The test suite fails on either Linux i386 or Win32
12. Handle locked pages more robustly (Fixes issue #1462)

    Memory locks do not stack, that is, pages which have been locked several times by calls to mlock()
    will be unlocked by a single call to munlock(). This can result in keying material ending up in swap when
    those functions are used naively. In this commit a class "LockedPageManager" is added
    that simulates stacking memory locks by keeping a counter per page.

    e95568b78d
13. Make CCrypter use LockedPageManager to manage locked pages

    Replace direct calls to mlock.
    
    Also, change the class to lock the memory areas in the constructor and unlock them again in the destructor. This makes sure that locked pages won't leak.

    0b886ad1bd
14. 
    BitcoinPullTester commented at 9:00 AM on August 23, 2012: none

    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/0b886ad1bda23c01a68d0ebeac9ec480ca5690fd for binaries and test log.

15. 
    gmaxwell commented at 11:05 PM on August 23, 2012: contributor

    Passes tests and WORKSFORME.

16. sipa referenced this in commit af1c6b93b7 on Aug 24, 2012
17. sipa merged this on Aug 24, 2012
18. sipa closed this on Aug 24, 2012

    ---

19. laanwj deleted the branch on Apr 9, 2014
20. suprnurd referenced this in commit 673e161d5d on Dec 5, 2017
21. DrahtBot locked this on Sep 8, 2021

Milestone
0.7.0