Opened 6 years ago

Closed 6 years ago

#11276 closed defect (fixed)

openssl_mutexes_ leaked from setup_openssl_threading()

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

On moria1, I found this leak:

==7518== 1,872 (312 direct, 1,560 indirect) bytes in 1 blocks are definitely lost in loss record 29 of 32 
==7518==    at 0x4C244E8: malloc (vg_replace_malloc.c:236)
==7518==    by 0x501A87: tor_malloc_ (util.c:162)
==7518==    by 0x50BC5A: crypto_early_init (crypto.c:3167)
==7518==    by 0x42B3FF: tor_init (main.c:2330)
==7518==    by 0x42C55A: tor_main (main.c:2859)
==7518==    by 0x5EFEC8C: (below main) (libc-start.c:228)

That line is:

  openssl_mutexes_ = tor_malloc(n*sizeof(tor_mutex_t *));

which appears to only be called from crypto_early_init(), which checks this to make sure it's never called more than once:

  if (!crypto_early_initialized_) {

But (ah ha), what sets crypto_early_initialized_ to 1 afterwards?

Child Tickets

Change History (4)

comment:1 Changed 6 years ago by arma

Status: newneeds_review

Suggested fix is in my bug11276 branch.

It's not tested and doesn't have a changes/ file.

Also, it would be good to ponder if anything was actually going more wrong by having crypto_early_init() run twice.

comment:2 Changed 6 years ago by nickm

Code looks okay.

Writing the changelog is indeed nontrivial. The items invoked in that function are:

  • ERR_load_crypto_strings(); -- ???
  • OpenSSL_add_all_algorithms(); -- ???
  • setup_openssl_threading(); -- ???
  • some log messages -- extra log messages are not a big deal
  • crypto_force_rand_ssleay(); -- idempotent
  • crypto_seed_rng(1) -- safe to seed as much as is requested
  • crypto_init_siphash_key() -- idempotent because of have_seeded_siphash variable

So it's just the ??? items above that need investigating FWICT

comment:3 Changed 6 years ago by arma

moria1 survived running with this patch for a while. The leak did not reoccur (but I haven't checked to see how rare it is).

comment:4 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged, plus a changes file. Thanks!

Note: See TracTickets for help on using tickets.