Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#8789 closed defect (fixed)

"err" target in connection_listener_new() should close socket s

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

Description

Right now, there are some paths out of connection_listener_new() where s is neither closed nor used in a new listener. They can only happen on error -- typically, an error that will result in an exiting Tor -- but we should still fix this.

Child Tickets

Attachments (1)

0001-Close-socket-at-err-target.patch (4.1 KB) - added by arlolra 7 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 years ago by nickm

Additionally, there may be fd-leak-on-error cases in add_file_log(), read_file_to_str(), and finish_daemon().

Changed 7 years ago by arlolra

comment:2 Changed 7 years ago by arlolra

Status: newneeds_review

Attached a patch for connection_listener_new().

comment:3 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged, with a slight tweak in b4b0063e48a2687e. Thanks!

comment:4 in reply to:  1 Changed 7 years ago by arlolra

Replying to nickm:

Additionally, there may be fd-leak-on-error cases in add_file_log(), read_file_to_str(), and finish_daemon().

I didn't look at this yet. Should the resolution be fixed?

comment:5 Changed 7 years ago by nickm

Resolution: fixed
Status: closedreopened

I looked at it again and didn't find any leaks. I'll stick the ticket in needs_review though, until somebody else has a look at those functions to see whether they leak.

comment:6 Changed 7 years ago by nickm

Status: reopenedneeds_review

comment:7 Changed 7 years ago by asn

Looks good to me!

comment:8 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:9 Changed 7 years ago by arma

I'm cleaning up the changes file here -- Nick/Arlo, can you point to the code paths that were wrong before? Or was this just a refactoring?

comment:10 Changed 7 years ago by nickm

Looks like a refactoring.

Note: See TracTickets for help on using tickets.