[tor-dev] Torsocks reengineered

[Mike Perry]

David Goulet:

> Hi everyone,
> 
> About a week ago I've sent an email to the Torsocks maintainers to
> address some concerns I had about important issues of the current code
> base. For the reasons found below, I proposed a rewrite that I am
> willing to do and submit it to the community once I feel ok with it.
> 
> Here is the repository I've created and started working on the rewrite.
> It has been branched from upstream master at commit
> e0bf65b5d3ca0e28b827c08d80b0fe1841d5a149
> 
> 	https://github.com/dgoulet/torsocks/tree/rewrite
> 
> Torsocks rewrite reasons:
> 
> 1) The code needs to be easier to maintain, so that it isn't
> sporadically maintained. Considering the number of issues (see below),
> it needs more love on a regular basis right now.
> 
> 2) It has never been safe from the start so a rewrite will NOT impact
> the current state of security. There is not much tests right now.
> 
> 4) One of the biggest technical issues is that it's not thread safe and
> fixing it right now would add an important impact on performance adding
> locks to multiple global data structures. A clean rewrite would allow to
> take into account this *very* important feature from the start without
> any ugly hacks in the current code base.

Please be extra careful with locking, especially since you're going to
redesign this.

I haven't personally reviewed torsock's current data structure use, but
in general, when somebody looks at a codebase and says "this code is not
threadsafe - it needs more locking around its data structure use", that
person is usually wrong, or at best only half right. It's almost always
better to get the locking out of your critical path and use thread local
storage, message passing, and/or job scheduling instead of direct lock
acquisition+release for commonly used data structures.

This is not just for performance reasons. Deadlocks are forever.

Library call hooking like this is especially tricky, because it could
easily lead you to subtle instances of lock order reversal deadlock if
you attempt to acquire your data structure lock before an OS/app lock
via one socket call path, but after it in another (for example, due to
OS/app abstractions that cause certain socket calls to be merely
wrappers for combinations of others).

Such app-specific and race-prone deadlock conditions will be very
annoying to track down. Careful design of how you deploy locking and
concurrency is a very wise time investment for this reason. You should
probably post a design doc and ask for review of it.

> 5) This is a very, and I can't stress this enough, _VERY_ intrusive
> library so extra care MUST be put in making it bullet proof in terms of
> error handling. Right now, there is multiple call sites that can fail on
> error.


-- 
Mike Perry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.torproject.org/pipermail/tor-dev/attachments/20130603/8feec966/attachment.pgp>