Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#3473 closed defect (fixed)

Better signal handling for obfsproxy

Reported by: asn Owned by: asn
Priority: Medium Milestone:
Component: Archived/Obfsproxy Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: #3591 Points:
Reviewer: Sponsor:

Description (last modified by nickm)

We currently treat SIGINT and SIGTERM equally, by terminating obfsproxy asap.

180-pluggable-transport.txt dictates that:

Proxies should respond to a single INT signal by closing their
listener ports and not accepting any new connections, but keeping
all connections open, then terminating when connections are all
closed. Proxies should respond to a second INT signal by shutting
down cleanly.

I think we should conform SIGINT with the 180 spec, and leave SIGTERM as is.

Child Tickets

Change History (9)

comment:1 Changed 8 years ago by asn

Status: newneeds_review

Please check out my bug3473 branch @ git://gitorious.org/obfsproxy/obfsproxy.git

(funky bold letters on the 'Description' of this entry were added accidentally.)

comment:2 Changed 8 years ago by nickm

Description: modified (diff)

Instead of (or in addition to) the shutting_down flag, I think it's better to close the listeners when we're shutting down, rather than just make them close their sockets immediately.

Did you test this? The cl_remove in conn_free() comes right after the memset that wrecks the contents of conn.

Why have a separate conn_list_t type; why not just add the 'next' pointer to conn_t?

For efficient removal, the list should probably be doubly-linked.

comment:3 Changed 8 years ago by asn

Thanks for the review.
Please check my bug3473 branch again for updates.

I implemented doubly linked lists and I now use them for connections and for listeners.

I also now disable listeners instead of closing their sockets. The thing with this is that disabling listeners doesn't FIN additional incoming connections, like closing the socket does, and terminating additional connections seems more appropriate in this case. What do you think?

comment:4 in reply to:  3 Changed 8 years ago by rransom

Replying to asn:

I also now disable listeners instead of closing their sockets. The thing with this is that disabling listeners doesn't FIN additional incoming connections, like closing the socket does, and terminating additional connections seems more appropriate in this case. What do you think?

obfsproxy should close its listening sockets, so that a new instance of obfsproxy (e.g. on a server) can start listening on the same ports immediately.

comment:5 Changed 8 years ago by nickm

Right; you don't want to disable the listeners (via evconnlistener_disable); you want to free them (which should close their sockets, if they were set up to close their sockets on free).

comment:6 Changed 8 years ago by asn

Updated.
Please check again.

comment:7 Changed 8 years ago by asn

Parent ID: #3591

comment:8 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged with some changes; please check them out. :)

comment:9 Changed 8 years ago by arma

Component: Pluggable transportObfsproxy
Note: See TracTickets for help on using tickets.