Terminate immediately when allocation fails #9856

1. 
   theuni commented at 5:30 am on February 25, 2017: member

   Addresses #9854.

   This installs a handler for failed allocations. Usually, a std::bad_alloc would be thrown, but we can instead opt to terminate immediately. See http://en.cppreference.com/w/cpp/memory/new/set_new_handler .

   Our only manual malloc/free comes from prevector. Ideally we’d just use new[]/delete[] there, but the benchmark shows a noticeable difference, and I’m very hesitant to make that kind of change post-rc2.

   As an alternative, this could even just be an assert() for 0.14.

   For reference, here’s the alternative using new/delete: https://github.com/theuni/bitcoin/commit/0c1b4107b918d457376dcd154f7e87d9d647bf91

   Edit: g++/libstdc++ didn’t support get_new_handler() yet, so assert it is.

2. fanquake added this to the milestone 0.14.0 on Feb 25, 2017
3. fanquake added the label Refactoring on Feb 25, 2017
4. 
   sipa commented at 5:44 am on February 25, 2017: member

   0./prevector.h:178:21: error: ‘get_new_handler’ is not a member of ‘std’
   1
   2                     std::get_new_handler()();
   

5. 
   theuni commented at 5:52 am on February 25, 2017: member

   @sipa grr, works for me locally. I’ll try adding the include, but I’m guessing that libstdc++ didn’t support it for 4.8 :(
6. theuni force-pushed on Feb 25, 2017
7. theuni force-pushed on Feb 25, 2017
8. don't throw std::bad_alloc when out of memory. Instead, terminate immediately c5f008a416
9. 
   JeremyRubin commented at 6:48 am on February 25, 2017: contributor

   Why not make prevector throw std::bad_alloc or std::length_error? That way it’s at least recoverable in future code, and makes it a bit more “API Consistent” with std::vector…
10. theuni force-pushed on Feb 25, 2017
11. 
    theuni commented at 7:21 am on February 25, 2017: member

    @JeremyRubin The issue in #9854 is that exactly that bad_alloc is thrown but not handled correctly. We don’t actually attempt to catch bad_alloc specifically anywhere. And logically it doesn’t make sense to… if we’ve got an allocation problem, we need to shut down.
12. 
    luke-jr commented at 7:30 am on February 25, 2017: member

    The default action of exceptions is to abort the program, no? So we must be catching it at least indirectly… Why not just add an explicit catch for bad_alloc there? (or ideally, be specific about what exceptions we want to catch..)
13. 
    laanwj commented at 7:36 am on February 25, 2017: member

    ACK on the assert() in prevector.

    I really don’t like setting this global handler that crashes the program. I hope this is a temporary measure for 0.14?

14. 
    laanwj commented at 7:38 am on February 25, 2017: member

    Thinking of it, no, using assert for error handling is a bad idea. We should support compiling without assertions at some point. If you really want to terminate the program immediatelly please log a message and call abort or so () :/
15. 
    TheBlueMatt commented at 8:00 am on February 25, 2017: member

    @laanwj I hope we do keep this longer-term, actually…std::bad_alloc, in the design of our entire codebase, seems massively dangerous to me (and I’ve wished we could do something like this for a while - just didn’t know it existed). I’m happy if we remove it later, but the amount of auditing we’d have to do to check everywhere we call new, even through stl, seems like a rather significant effort.

    On February 25, 2017 2:36:44 AM EST, “Wladimir J. van der Laan” notifications@github.com wrote:

    ACK on the assert() in prevector.

    I really don’t like setting this global handler that crashes the program. I hope this is a temporary measure for 0.14?

    – You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9856#issuecomment-282467342

16. 
    laanwj commented at 8:32 am on February 25, 2017: member

    @TheBlueMatt okay, yes, after discussion IRC I agree seems the least bad solution for now :/

    Edit: so to be clear, my concern with a jackhammer solution like this is that not all allocations are created equal. An allocation of a 4 GB buffer somewhere in the program may fail, but that may be perfectly fine if it can continue with a smaller buffer. It doesn’t mean the rest of the application, which will never allocate a big buffer like that, is in a bad state. Specificallly we care deeply about bad_allocs thrown by consensus and block/utxo handling code. Error handling in Bitcoin Core has always been a bit flakey, and in a way his feels a bit like throwing in the towel on reasoned error handling completely.So that’s why my angry reply. Sorry for that. But we need this now to avoid corruption problems. I think we should continue with this.

17. 
    JeremyRubin commented at 9:10 am on February 25, 2017: contributor

    Ah, I see. I was going to suggest get_new_handler, then I actually read the above discussion ;)

    I’m mostly just uncomfortable with using assert for error handling.

    Can we not implement something similar to get_new_handler ourselves for unsupported platforms?

18. 
    TheBlueMatt commented at 5:38 pm on February 25, 2017: member

    @laanwj yea, I totally agree there are places we don’t want this (mostly deserialization comes to mind), but I believe we mostly fixed that by now?

    On February 25, 2017 4:10:18 AM EST, Jeremy Rubin notifications@github.com wrote:

    Ah, I see. I was going to suggest get_new_handler, then I actually read the above discussion ;)

    I’m mostly just uncomfortable with using assert for error handling.

    Can we not implement something similar to get_new_handler ourselves for unsupported platforms?

    – You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9856#issuecomment-282471576

19. in src/init.cpp: in 420f170c21 outdated

    802+    // Rather than throwing std::bad-alloc if allocation fails, terminate
    803+    // immediately to (try to) avoid chain corruption.
    804+    // Since LogPrintf may itself allocate memory, set the handler directly
    805+    // to terminate first.
    806+    std::set_new_handler(std::terminate);
    807+    LogPrintf("Error: Out of memory. Terminating.\n");
    

    ---

    ---

    

    kallewoof commented at 1:45 am on February 26, 2017:

    Before this attempt, you could do fputs(stderr, "Error: Out of memory. Terminating.\n"); which, I believe, does not call malloc. At least it didn’t when I wrote my own malloc a few years ago, but it may be different per platform. A nice to have for when a node goes down with no log entries about why.

    ---

    

    JeremyRubin commented at 11:36 am on February 26, 2017:

    I don’t think this is threadsafe (ie, a concurrent OOM could cause it to terminate in the middle of a LogPrintf or print two LogPrintfs). I would make the handler have some switch(atomic counter++). Then it is both re-entrant and threadsafe. (note that set_new_handler is threadsafe itself, just is threadsafe in this usage)

    I think this sort of implementation should be ok:

     0static std::thread::id first_entrant{std::this_thread::get_id()};
     1static bool first_pass {true};
     2if (first_entrant == std::this_thread::get_id()) {
     3    if (first_pass) {
     4        first_pass = false;
     5        // Single faulted
     6        std::terminate();
     7    } else {
     8        // Double faulted
     9    }
    10} else {
    11    sleep();
    12}
    

    Either of the above should be safe for concurrent OOM as well as double OOM faults.

    ---

    

    theuni commented at 7:41 pm on February 27, 2017:

    Those are both reasonable suggestions, but imo they’re overthinking this a bit. If we OOM, the important thing is to shutdown, logging should be best-effort. I’d rather not complicate this too much.
20. dcousens approved
21. 
    sdaftuar commented at 3:30 pm on February 27, 2017: member

    nit: Perhaps we should add a comment to the assert lines introduced in prevector, mentioning their purpose/rationale? (I’m pretty sure if I were reading the code in the future and hadn’t seen this discussion, I’d definitely overlook the significance and implications.)
22. theuni force-pushed on Feb 27, 2017
23. prevector: assert successful allocation d4ee7baef7
24. 
    sipa commented at 8:37 am on February 28, 2017: member

    utACK
25. laanwj merged this on Feb 28, 2017
26. laanwj closed this on Feb 28, 2017

    ---

27. laanwj referenced this in commit 65fdc37ac3 on Feb 28, 2017
28. laanwj referenced this in commit 69832aaad5 on Feb 28, 2017
29. laanwj referenced this in commit 775cf54d0e on Feb 28, 2017
30. codablock referenced this in commit 5c7c3b3a00 on Jan 26, 2018
31. str4d referenced this in commit 00e07a2cbb on Apr 23, 2018
32. str4d referenced this in commit 3c9dbf3ed8 on Apr 26, 2018
33. zkbot referenced this in commit 1fd3207b9e on May 1, 2018
34. zkbot referenced this in commit 23f8b30c88 on May 1, 2018
35. lateminer referenced this in commit 5a55d6f015 on Jan 4, 2019
36. andvgal referenced this in commit 44f37e0f83 on Jan 6, 2019
37. CryptoCentric referenced this in commit 32871e925e on Feb 27, 2019
38. Fuzzbawls referenced this in commit 8dfc4806f7 on May 19, 2020
39. DrahtBot locked this on Sep 8, 2021

Contributors
theuni sipa JeremyRubin luke-jr laanwj TheBlueMatt kallewoof dcousens sdaftuar

Labels
Refactoring

Milestone
0.14.0