Opened 5 months ago

Closed 5 months ago

Last modified 7 days ago

#27246 closed defect (implemented)

Can we use less space for RSA onion keys on clients?

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 035-roadmap-master, 035-triaged-in-20180711
Cc: Actual Points: .5
Parent ID: #27243 Points:
Reviewer: nickm Sponsor: Sponsor8

Description

Our heap profiles say that we're using a HUGE amount of storage for RSA onion keys. That's pretty sad, considering that we only use them for legacy v2 hidden service stuff.

Maybe we can only parse them on-demand, when we need them?

Maybe (if the openssl format is much more expensive than the raw asn1) we can store them in asn1, and only convert them to EVP_PKEY objects when we need them?

Child Tickets

Change History (10)

comment:1 Changed 5 months ago by nickm

I can confirm that there's a blow-up here -- I ran an experiment with openssl to see how much space it uses to store an RSA that it has loaded from an ASN1 string.

The asn.1 string was 140 bytes long. OpenSSL allocated a total of 408 bytes in order to hold the RSA object and its lock, not counting the malloc headers. (It grabbed one RSA object, one lock, 2 BIGNUM objects, and 2 bitvectors inside the BIGNUMs.)

comment:2 Changed 5 months ago by dgoulet

Status: newneeds_review

Here is an attempt: ticket27246_035_01
PR: https://github.com/torproject/tor/pull/292

Test passes so I'm optimistic.

comment:3 Changed 5 months ago by dgoulet

Reviewer: nickm

comment:4 Changed 5 months ago by nickm

In experimental context: we used 5.9MB out of 24.7MB total for storing crypto_pk_t and its contents. If we can cut that down by a factor of (140/408), we'll save 3.9 MB, or 15% of our total memory used -- not counting the malloc overhead!

comment:5 Changed 5 months ago by nickm

Status: needs_reviewneeds_revision

Nice! I expect that the memory savings will be even better than expected in the commit message, since the "408 vs 140" calculation doesn't include malloc overhead.

I suggested a few small changes on the ticket, and found (what I think is) one real bug.

Question: Have you tested this for memory leaks? I didn't see any, but that would be the kind of bug I'd want to watch out for here.

comment:6 in reply to:  5 Changed 5 months ago by dgoulet

Status: needs_revisionneeds_review

Replying to nickm:

Nice! I expect that the memory savings will be even better than expected in the commit message, since the "408 vs 140" calculation doesn't include malloc overhead.

I suggested a few small changes on the ticket, and found (what I think is) one real bug.

Question: Have you tested this for memory leaks? I didn't see any, but that would be the kind of bug I'd want to watch out for here.

I have not run this under valgrind but no leaks occur in the test. There are a LOT of code path in the unit tests that touches the md/ri and I know because I had a leak during development and noticed very quickly.

See fixup commits on the PR.

comment:7 Changed 5 months ago by nickm

Status: needs_reviewmerge_ready

Fixups LGTM; will squash and merge after CI is done.

comment:8 Changed 5 months ago by nickm

Squashed, added a changes file and some comments, merged.

comment:9 Changed 5 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

comment:10 Changed 7 days ago by nickm

Actual Points: .5
Note: See TracTickets for help on using tickets.