Teach Tor to control networks with tor-fw-helper.c

changed milestone to %Deliverable-Sep2010

added component::core tor/tor milestone::Deliverable-Sep2010 parent::1775 priority::medium resolution::implemented status::closed type::enhancement upnp version::tor unspecified labels

Okay, so I'm reviewing sjmurdoch's "upnp" branch, which looks like it contains all of ieorror's tor-fw-helper branch. Some of this will apply to this ticket and some will apply to #1900 (moved). I'll try to put the right comments on the right ticket, but I might make a mistake, so you should probably look in both places.

This review is based on "git diff master...sjm217/upnp"; I'm looking at the overall changes, not the individual patches. The current sjm217/upnp head I'm looking at is b86cbcc67f1.

In zlib.c: It looks like some old bug1526 stuff snuck into this branch. We should kill that when we finally rebase and squash (probably we shouldn't rebase and squash until we are for-sure ready to merge, though).

In util.c: format_helper_exit_status looks safe, but I'd be a little more comfortable if there were more explicit checks to make sure we can't run off the start of the buffer. You check in the two loops, but not before adding the '-' and '/'. The Doxygen comment should require that hex_errno have at least HEX_ERRNO_SIZE bytes available. Also, the format should be documented somewhere; if not in the doc for format_helper_exit_status, then somewhere mentioned in that doc.

Also on format_helper_exit_status: it seems to me that the caller is responsible for filling the buffer with ' ' characters and appending a trailing '\n\0'. Why not put the responsibility into format_helper_exit_status?

In tor_spawn_background: stdout_read, stderr_read, and argv aren't documented. Also, it would be good to note (via an XXXX comment, or ideally a trac ticket) that we could remove the icky for(fd=STDERR_FILENO+1;fd<max_fd; fd++) close(fd) loop by setting FD_CLOEXEC on all of our files.

It still needs a Windows implementation.

log_err is for nonsurvivable errors; log_warn is more appropriate for a case where you can't close an fd. (This applies to log_from_pipe too).

It would be very good to have a unit test for this.

In log_from_pipe():

strstr is for telling you if a string is in a buffer, but the comment above strstr(buf, SPAWN_ERROR_MESSAGE) implies that we want to know if SPAWN_ERROR_MESSAGE is at the start of the buffer. For that, use strcmpstart().

Prefer tor_sscanf to sscanf whenever possible.

I'm confused by the tor_sscanf format: first off, if you want to check for an exact match rather then a prefix match, you need to say 'r = sscanf("%d/%d%c", &i1,i2,&c)' and make sure that r==2 (not 0 or 1 or 3). But why is the format looking for %d? Is it scanning the output of format_helper_exit_status? That function encodes things as hexadecimal, not decimal.

What does fgets do when read() returns an EAGAIN in the middle of a line? Does it return a partial line, or wait for a newline?

tor_check_port_forwarding: is missing documentation. Because of that, it took me a little while to figure out that you're supposed to call it over and over.

It seems that the code here does two things: one is a general task "Launch a child process and see what it says" and another is a more specific task "Launch a tor-fw-helper instance and act based on its output.)" It might be a good idea to disentangle these eventually, in case we ever want to launch anything else.

It might be cleaner to move the static variables from tor_check_port_forwarding into some kind of struct, in case we ever want to launch two things in the future.

Trac:
Status: new to needs_review

The stuff I reviewed before is now in master; we should still fix the above bugs, though.

Nick - you said:

In zlib.c: It looks like some old bug1526 stuff snuck into this branch. We should kill that when we finally rebase and squash (probably we shouldn't rebase and squash until we are for-sure ready to merge, though).

Is that still true? I know that we did a rebase and squash - did you handle this?

Ok - sjm says that you did handle the #1526 (moved) stuff. Yay.

''I'm going to summarize the issues Nick mentioned into numbers as subtasks for this bug: ''

In util.c: format_helper_exit_status looks safe, but I'd be a little more comfortable if there were more explicit checks to make sure we can't run off the start of the buffer. You check in the two loops, but not before adding the '-' and '/'. The Doxygen comment should require that hex_errno have at least HEX_ERRNO_SIZE bytes available. Also, the format should be documented somewhere; if not in the doc for format_helper_exit_status, then somewhere mentioned in that doc.
Also on format_helper_exit_status: it seems to me that the caller is responsible for filling the buffer with ' ' characters and appending a trailing '\n\0'. Why not put the responsibility into format_helper_exit_status?
In tor_spawn_background: stdout_read, stderr_read, and argv aren't documented. Also, it would be good to note (via an XXXX comment, or ideally a trac ticket) that we could remove the icky for(fd=STDERR_FILENO+1;fd<max_fd; fd++) close(fd) loop by setting FD_CLOEXEC on all of our files.
It still needs a Windows implementation.
log_err is for nonsurvivable errors; log_warn is more appropriate for a case where you can't close an fd. (This applies to log_from_pipe too).
It would be very good to have a unit test for this.
_In log_from_pipe(): _
strstr is for telling you if a string is in a buffer, but the comment above strstr(buf, SPAWN_ERROR_MESSAGE) implies that we want to know if SPAWN_ERROR_MESSAGE is at the start of the buffer. For that, use strcmpstart().
Prefer tor_sscanf to sscanf whenever possible.
I'm confused by the tor_sscanf format: first off, if you want to check for an exact match rather then a prefix match, you need to say 'r = sscanf("%d/%d%c", &i1,i2,&c)' and make sure that r==2 (not 0 or 1 or 3). But why is the format looking for %d? Is it scanning the output of format_helper_exit_status? That function encodes things as hexadecimal, not decimal.
What does fgets do when read() returns an EAGAIN in the middle of a line? Does it return a partial line, or wait for a newline?
tor_check_port_forwarding: is missing documentation. Because of that, it took me a little while to figure out that you're supposed to call it over and over.
It seems that the code here does two things: one is a general task "Launch a child process and see what it says" and another is a more specific task "Launch a tor-fw-helper instance and act based on its output.)" It might be a good idea to disentangle these eventually, in case we ever want to launch anything else.
It might be cleaner to move the static variables from tor_check_port_forwarding into some kind of struct, in case we ever want to launch two things in the future.

Issue number 4 is handled by Ticket #1983 (moved) - it will happen at a later date.

I've handled issue number 12 in commit [tor-fw-helper 48245af] document tor_check_port_forwarding

Nick,

Thanks for your comments, I am working through them and my results can be found in the bug1903 branch of my Tor repository: git://git.torproject.org/sjm217/tor.git (N.B. I may rebase it, so it is not ready for merge yet). Most of your comments are straightforward to fix, but I have a question about one.

11. What does fgets do when read() returns an EAGAIN in the middle of a line? Does it return a partial line, or wait for a newline?

I thought the latter, but the unit test I just added (dd66163) shows that it is the former :-( It looks like I need to write my own function to read a line from a non-blocking file descriptor.

Alternatively, I did find fetch_from_buf_line() in buffers.c, but this deals with a buffer rather than a pipe. As far as I can tell, buffers.c uses recv() to read data rather than read(), which I don't think is guaranteed to work on a pipe.

I am not sure which is the best way forward, so do you have a suggestion?

Hrm. First thing to do here is to verify that Windows will even let you make a pipe (made with CreatePipe) nonblocking. If not, let's step back and think of a more general solution here.

Assuming that the general solution is indeed to do nonblocking IO on the pipes (or that there is no general solution, and nonblocking IO on pipes is the Unix solution):

recv is indeed only for sockets.
This doesn't need to be super-efficient. Maybe add to the per-process state a smartlistlist of strings that have been read from fgets() before the current call to the function that gets a line, and when we hit EAGAIN, add the string to the list. When we finally hit a newline, prepend the concatenation of all the strings in the smartlist to what we got.
- This also suggests to me that we might want to have a struct associated with each process we've launched; the number of related things that we need to pass around or hold for each one seems to be growing.

Plausible?

Replying to nickm:

Hrm. First thing to do here is to verify that Windows will even let you make a pipe (made with CreatePipe) nonblocking. If not, let's step back and think of a more general solution here.

Good point. What I will do in that case is to shelve this issue in bug #2045 (moved) and start on the Windows port in #1983 (moved). Once I have done some research on how to handle I/O redirection on Windows, I will have a better idea on how to solve this problem in a cross-platform manner.

Replying to nickm:

I now think I have either dealt with or created new tickets for all the issues you have pointed out. My changes have been put in branch bug1903 of git://git.torproject.org/sjm217/tor.git (8a12ce2). I think this should be ready for merge, with the possible exception of a unit test failing as a result of bug #2045 (moved). Should I disable this test until I have fixed the bug?

The other issues dealt with are:

In util.c: format_helper_exit_status looks safe, but I'd be a little more comfortable if there were more explicit checks to make sure we can't run off the start of the buffer. You check in the two loops, but not before adding the '-' and '/'. The Doxygen comment should require that hex_errno have at least HEX_ERRNO_SIZE bytes available. Also, the format should be documented somewhere; if not in the doc for format_helper_exit_status, then somewhere mentioned in that doc.

Done in 5a77c64.

Also on format_helper_exit_status: it seems to me that the caller is responsible for filling the buffer with ' ' characters and appending a trailing '\n\0'. Why not put the responsibility into format_helper_exit_status?

I did that because I couldn't put the memset in there, because I was not sure it could be guarenteed to be signal handler safe. I agree with your point so have replaced it with a for loop (in 5a77c64).

In tor_spawn_background: stdout_read, stderr_read, and argv aren't documented.

Done in 68e576e.

Also, it would be good to note (via an XXXX comment, or ideally a trac ticket) that we could remove the icky for(fd=STDERR_FILENO+1;fd<max_fd; fd++) close(fd) loop by setting FD_CLOEXEC on all of our files.

Noted in 708ba88 and will be dealt with in bug #2029 (moved).

It still needs a Windows implementation.

This is bug #1983 (moved).

log_err is for nonsurvivable errors; log_warn is more appropriate for a case where you can't close an fd. (This applies to log_from_pipe too).

Done in 4d694c7.

It would be very good to have a unit test for this.

Done in 8a12ce2.

In log_from_pipe(): strstr is for telling you if a string is in a buffer, but the comment above strstr(buf, SPAWN_ERROR_MESSAGE) implies that we want to know if SPAWN_ERROR_MESSAGE is at the start of the buffer. For that, use strcmpstart().

I was checking whether strstr(s1, s2) == s1, not != NULL, so I think it does check if it is at the start. In any case, I've replaced it with strcmpstart() which makes it clearer what functionality is intended (and may be marginally faster), in 23e9f36.

Prefer tor_sscanf to sscanf whenever possible.

Done in 23e9f36.

I'm confused by the tor_sscanf format: first off, if you want to check for an exact match rather then a prefix match, you need to say 'r = sscanf("%d/%d%c", &i1,i2,&c)' and make sure that r==2 (not 0 or 1 or 3).

I thought I did do this by "if (retval == 2) {..." a couple of lines later.

But why is the format looking for %d? Is it scanning the output of format_helper_exit_status? That function encodes things as hexadecimal, not decimal.

Indeed, that's a bug (fixed in 23e9f36).

What does fgets do when read() returns an EAGAIN in the middle of a line? Does it return a partial line, or wait for a newline?

This will be dealt with in bug #2045 (moved) (see comment:8 above.)

tor_check_port_forwarding: is missing documentation. Because of that, it took me a little while to figure out that you're supposed to call it over and over.

ioerror documented it in 48245af of git://git.torproject.org/ioerror/tor.git.

It seems that the code here does two things: one is a general task "Launch a child process and see what it says" and another is a more specific task "Launch a tor-fw-helper instance and act based on its output.)" It might be a good idea to disentangle these eventually, in case we ever want to launch anything else.

It might be cleaner to move the static variables from tor_check_port_forwarding into some kind of struct, in case we ever want to launch two things in the future.

As this seems less urgent than the other tasks, I have noted these in bug #2030 (moved).