[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #1903 [Tor Client]: Teach Tor to control networks with tor-fw-helper.c
#1903: Teach Tor to control networks with tor-fw-helper.c
-------------------------+--------------------------------------------------
Reporter: ioerror | Owner:
Type: enhancement | Status: needs_review
Priority: normal | Milestone: Deliverable-Sep2010
Component: Tor Client | Version: Tor: unspecified
Keywords: upnp | Parent: #1775
-------------------------+--------------------------------------------------
Comment(by ioerror):
''I'm going to summarize the issues Nick mentioned into numbers as
subtasks for this bug:
''
1. ''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.''
1. ''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?''
1. ''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.''
1. ''It still needs a Windows implementation.''
1. ''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).''
1. ''It would be very good to have a unit test for this.''
1. ''In log_from_pipe():Â''
1. ''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().''
1. ''Prefer tor_sscanf to sscanf whenever possible.''
1. ''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.''
1. ''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?''
1. ''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.''
1. ''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.''
1. ''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.''
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/1903#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs