Trivial issues in tor_spawn_background()/tor_check_port_forwarding()
Some things I noticed while coding #3472 (moved) 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.
Activity
changed milestone to %Tor: 0.2.3.x-final
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.
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!
-
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.
-
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
95df581for now. -
Aha! I see that your
d6c18c5804f12f8f4428abce11fc47c64d2a0dd0stoppedfclose()ing the handles inlog_from_pipe()for a good reason.I guess the new question is, who calls
tor_process_handle_destroy()on thechild_handlecreated by the lasttor_check_port_forwarding()call? I guess no one.Do we care about this? If not, I'm all about closing this stupid ticket.
-
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 terminationprocess_handle_destroy()is not called on thechild_handle. This leaks theprocess_handle_tand 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 exposingchild_handleas a file-level static variable (renamed asport_fw_child_handle) and proving aport_forwarding_free_all()function be called upon shutdown could help... Still, that fix is kinda messy.