Opened 2 weeks ago

Closed 6 days ago

#27451 closed defect (fixed)

Refactor socket closing in tor_tls_free() so it is not broken with NSS

Reported by: nickm Owned by: nickm
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: nickm, gk Actual Points:
Parent ID: #26631 Points:
Reviewer: Sponsor:

Description

tor_tls_free() closes the socket in openssl, but not in nss. This is asking for trouble.

Child Tickets

Change History (5)

comment:1 Changed 2 weeks ago by nickm

My first attempt at addressing this was in b5fddbd24144a94580e50886cd07a82968a1b86c, but that caused bug #27500, and had to be reverted with 22e24031452d57852e83738bacfff012439f0258.

I see two possible solutions here:

  • Find some way to make NSS follow OpenSSL's behavior, and let us free the connection without closing the fd.
  • Have OpenSSL follow NSS's behavior (which will be easy) -- and adjust our connection code to tolerate that behavior. (This is what I tried to do before).

comment:2 Changed 7 days ago by nickm

Status: assignedneeds_review

I have a patch here as nss_27451, with PR at https://github.com/torproject/tor/pull/322 .

It takes a hybrid approach: by default, the TLS connection object owns the socket and closes it when it's freed. But if something else needs to free the socket, ownership is wrested from the TLS connection object.

The unit tests pass for me, but chutney+nss is failing because of #27664.

comment:3 Changed 6 days ago by nickm

Summary: Refactor socket closing in tor_tls_free()Refactor socket closing in tor_tls_free() so it is not broken with NSS

comment:4 Changed 6 days ago by ahf

Status: needs_reviewmerge_ready

LGTM.

comment:5 Changed 6 days ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged!

Note: See TracTickets for help on using tickets.