No need to use OpenSSL malloc/free #10058

pull tjps wants to merge 1 commits into bitcoin:master from tjps:tjps_ssl_mutex changing 1 files +7 −10

tjps commented at 10:49 PM on March 22, 2017: contributor

The set of objects used in the OpenSSL locking callback do not need to be allocated using the OpenSSL allocator.
fanquake added the label Utils and libraries on Mar 22, 2017
fanquake commented at 10:53 PM on March 22, 2017: member

Please be aware the we use the [trivial] label for changes that are essentially non-code changes. i.e fixing typos. This PR would not be considered trivial.
fanquake renamed this:
~~[trivial] No need to use OpenSSL malloc/free~~
No need to use OpenSSL malloc/free
on Mar 22, 2017
tjps commented at 11:18 PM on March 22, 2017: contributor

Ack. will remove the trivial marker when I update the PR for the apparently broken test.
in src/util.cpp:165 in 0765b6c9f0 outdated
```
 158 | @@ -160,9 +159,8 @@ class CInit
 159 |          RAND_cleanup();
 160 |          // Shutdown OpenSSL library multithreading support
 161 |          CRYPTO_set_locking_callback(NULL);
 162 | -        for (int i = 0; i < CRYPTO_num_locks(); i++)
 163 | -            delete ppmutexOpenSSL[i];
 164 | -        OPENSSL_free(ppmutexOpenSSL);
```
jonasschnelli commented at 7:25 AM on March 23, 2017:

~~AFAIK it was slightly wrong to free the ppmutexOpenSSL via OPENSSL_free. IMO only objects alloced by OPENSSL_malloc() should use OPENSSL_free(). This change makes sense.~~ I was wrong: It is currently (master) alloced with OPENSSL_malloc.
laanwj commented at 7:32 AM on March 23, 2017:

It is allocated via OPENSSL_malloc

ppmutexOpenSSL = (CCriticalSection**)OPENSSL_malloc(CRYPTO_num_locks() * sizeof(CCriticalSection*));
jonasschnelli commented at 7:35 AM on March 23, 2017:

It is allocated via OPENSSL_malloc

Oh. Right.. it was. Nervermind then.
jonasschnelli commented at 7:26 AM on March 23, 2017: contributor

utACK 0765b6c9f08c5bac848ba8ac7c03471513fa4654
tjps commented at 9:52 PM on March 28, 2017: contributor

Bump, for one less OpenSSL eyesore

in src/util.cpp:120 in 0765b6c9f0 outdated

 116 | @@ -117,7 +117,7 @@ std::atomic<bool> fReopenDebugLog(false);
 117 |  CTranslationInterface translationInterface;
 118 |  
 119 |  /** Init OpenSSL library multithreading support */
 120 | -static CCriticalSection** ppmutexOpenSSL;
 121 | +static std::vector<std::unique_ptr<CCriticalSection>> ppmutexOpenSSL;

JeremyRubin commented at 10:05 PM on March 28, 2017:

Probably cleaner to just use a std::unique_ptr<CCriticalSection[]>

No need to use OpenSSL malloc/free 6d5dd60c88

in src/util.cpp:138 in 0765b6c9f0 outdated

 136 |      {
 137 |          // Init OpenSSL library multithreading support
 138 | -        ppmutexOpenSSL = (CCriticalSection**)OPENSSL_malloc(CRYPTO_num_locks() * sizeof(CCriticalSection*));
 139 |          for (int i = 0; i < CRYPTO_num_locks(); i++)
 140 | -            ppmutexOpenSSL[i] = new CCriticalSection();
 141 | +            ppmutexOpenSSL.emplace_back(new CCriticalSection());

JeremyRubin commented at 10:07 PM on March 28, 2017:

and then here ppmutexOpenSSL.reset(new CCriticalSection[CRYPTO_num_locks()]); I think is correct.

tjps commented at 12:36 AM on March 29, 2017:

You are correct. I always like shaving lines off.

sipa commented at 1:14 AM on March 29, 2017: member

utACK 6d5dd60c881b790f1b5fc5ba470605b6bd7e2fe7
laanwj merged this on Apr 3, 2017
laanwj closed this on Apr 3, 2017
laanwj referenced this in commit a2cd0b0eec on Apr 3, 2017
tjps deleted the branch on May 1, 2017
PastaPastaPasta referenced this in commit 33142b89df on May 10, 2019
PastaPastaPasta referenced this in commit 6515a17e54 on May 15, 2019
PastaPastaPasta referenced this in commit 1988088949 on May 20, 2019
PastaPastaPasta referenced this in commit 9b926c274b on May 21, 2019
barrystyle referenced this in commit 7ffa05e4e7 on Jan 22, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels