Opened 9 years ago

Closed 3 years ago

#4296 closed defect (fixed)

Trivial issues in tor_spawn_background()/tor_check_port_forwarding()

Reported by: asn Owned by:
Priority: Low Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, tor-client
Cc: nickm, sjmurdoch Actual Points:
Parent ID: Points: small
Reviewer: Sponsor:

Description

Some things I noticed while coding #3472 stuff:

  • In tor_spawn_background():
      retval = pipe(stderr_pipe);
      if (-1 == retval) {
    

if pipe() fails, the file descriptors of pipe(stdout_pipe) above are never closed.

  • In the tor_check_port_forwarding() code or in the Windows version of tor_read_all_handle() the stdout/stderr handles are not CloseHandle()d on stream error.

I'll prepare a patch.

Child Tickets

Change History (29)

comment:1 Changed 9 years ago by asn

Status: newneeds_review

Check out branch bug4296 in git://gitorious.org/mytor/mytor.git.

comment:2 Changed 9 years ago by nickm

95df581c054595 looks okay, but I don't know about 859b279a5cf25. In 859b279a5cf25, it looks like we are only calling CloseHandle on that one path through log_from_handle, but we aren't closing it on the other paths. I would expect that either all paths should close it, or none should, or that the documentation would explain why that sometimes-close behavior was reasonable.

comment:3 Changed 9 years ago by asn

In 859b279a5cf25, I tried to replicate the behavior of the Unix log_from_pipe() function which fclose()s the stream on error.

I think in the future log_from_{pipe,handle} should raise a flag on error and let the caller close the stream as they like.

I'm not feeling familiar enough with the port forwarder spawning code to do this modification myself.

I documented the reference stealing behavior in f5a0813a, pull away!

comment:4 Changed 9 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:5 Changed 9 years ago by nickm

Hrm. I still think 95df581 is good to merge, but for the others, we should indeed have the caller close stuff as appropriate. Having a "read" function that sometimes closes the pipe doesn't make much sense.

Should I cherrypick the one commit I like at this point? Looking at log_from_pipe(), it looks like it doesn't fclose streams any more.

comment:6 in reply to:  5 Changed 9 years ago by asn

Replying to nickm:

Hrm. I still think 95df581 is good to merge, but for the others, we should indeed have the caller close stuff as appropriate. Having a "read" function that sometimes closes the pipe doesn't make much sense.

Should I cherrypick the one commit I like at this point? Looking at log_from_pipe(), it looks like it doesn't fclose streams any more.

Sure, let's just cherrypick 95df581 for now.

comment:7 Changed 9 years ago by nickm

Status: needs_reviewneeds_revision

Okay; cherry-picked that one. Marking this needs-revision, but feel free to close if you think there's nothing left to revise.

comment:8 Changed 8 years ago by asn

Aha! I see that your d6c18c5804f12f8f4428abce11fc47c64d2a0dd0 stopped fclose()ing the handles in log_from_pipe() for a good reason.

I guess the new question is, who calls tor_process_handle_destroy() on the child_handle created by the last tor_check_port_forwarding() call? I guess no one.

Do we care about this? If not, I'm all about closing this stupid ticket.

comment:9 Changed 8 years ago by nickm

"this" being the resource leak that happens if nobody calls the destroy function? I do in fact care about resource leaks, if I understand you correctly.

comment:10 in reply to:  9 Changed 8 years ago by asn

Replying to nickm:

"this" being the resource leak that happens if nobody calls the destroy function? I do in fact care about resource leaks, if I understand you correctly.

tor_check_port_forwarding() is called periodically. The last time it is called before Tor termination process_handle_destroy() is not called on the child_handle. This leaks the process_handle_t and two file descriptors upon Tor termination. It's not a resource leak that stacks up.

The logic of tor_check_port_forwarding() is not very welcoming to fixes either; it does too many things. Maybe exposing child_handle as a file-level static variable (renamed as port_fw_child_handle) and proving a port_forwarding_free_all() function be called upon shutdown could help... Still, that fix is kinda messy.

comment:11 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

Okay, so if it's a leak-at-close, it doesn't need to be fixed in 0.2.3.

comment:12 Changed 8 years ago by nickm

Keywords: easy added

comment:13 Changed 8 years ago by nickm

Keywords: tor-client added

comment:14 Changed 8 years ago by nickm

Component: Tor ClientTor

comment:15 Changed 8 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

comment:16 Changed 7 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

comment:17 Changed 6 years ago by nickm

Keywords: 026-triaged-1 added

comment:18 Changed 6 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.7.x-final

comment:19 Changed 5 years ago by nickm

Keywords: 027-triaged-1-out added

Marking triaged-out items from first round of 0.2.7 triage.

comment:20 Changed 5 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.???

Move *most* 0.2.7-triaged-1-out needs_revision items into 0.2.???. Keep a few based on my sense of the sensible.

comment:21 Changed 5 years ago by nickm

Points: small
Priority: MediumLow
Severity: Normal

comment:22 Changed 4 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:23 Changed 4 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:24 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:25 Changed 3 years ago by nickm

Keywords: 027-triaged-in added

comment:26 Changed 3 years ago by nickm

Keywords: 027-triaged-in removed

comment:27 Changed 3 years ago by nickm

Keywords: 027-triaged-1-out removed

comment:28 Changed 3 years ago by nickm

Keywords: 026-triaged-1 removed

comment:29 Changed 3 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.3.x-final
Resolution: fixed
Status: needs_revisionclosed

Closing this as mostly merged; opened #22382 to track the remaining fd leak-on-exit.

Note: See TracTickets for help on using tickets.