Opened 8 months ago

Closed 7 months ago

#25150 closed enhancement (fixed)

Avoid malloc/free on each server-side ntor handshake

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: c99, malloc, performance, ntor, review-group-32
Cc: starlight@… Actual Points:
Parent ID: #23777 Points:
Reviewer: isis Sponsor:

Description

Now that we require c99, we can use variable-length array here instead of a malloc/free pair.

Child Tickets

Change History (7)

comment:1 Changed 8 months ago by nickm

Status: assignedneeds_review

Branch onion_ntor_malloc_less in my public repository.

comment:2 Changed 8 months ago by dgoulet

Hmm this fails with fragile hardening. I expect any distro building with this might end up not able to compile tor :S.

src/or/onion.c:532:1: error: stack protector not protecting local variables: variable length buffer [-Werror=stack-protector]
 onion_skin_server_handshake(int type,

I've looked at this and there are two callsites for onion_skin_server_handshake that both passes a stack pointer and a fixed length.

Maybe we could modify the callers to pass a full buffer containing the keys + nonce instead?

comment:3 Changed 7 months ago by nickm

Keywords: review-group-32 added

comment:4 Changed 7 months ago by isis

Reviewer: isis

Assigning tickets from review-group-32 to myself for review this week.

comment:5 Changed 7 months ago by isis

Status: needs_reviewmerge_ready

This change looks fine to me. I looked into why it might be breaking with --enable-fragile-hardening (which our Travis builds also use) and it appears to be an issue with clang's LeakSanitizer detecting the leaks from #25127 (the other GCC builds and non-Rust builds completed just fine). After rebasing it on top of the #25127 fix, clang seems happy.

comment:6 Changed 7 months ago by nickm

I've reproduced the fragile-hardening thing; it appears to be GCC-only though.

I've merged, and made a change to fix it, as 4d994e7a9c8936c9e33df90b7468e7327f1794e9

comment:7 Changed 7 months ago by nickm

Resolution: fixed
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.