[tor-dev] Torsocks reengineered

[David Goulet]

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.

Of course. :)

> 
> 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.

That makes sense. I'm not sure yet how complex will be the "locking model" given
that there is simply none right now, I'll surely submit a design proposal if it
becomes non trivial.

I come from a Linux kernel and RCU
(https://en.wikipedia.org/wiki/Read-copy-update) background so I'm used to
heavily document any locking scheme and create design proposal before making any
production code. So, I agree 100% with you on the importance of thinking this
through!

Thanks!
David

> 
>> 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.
> 
> 
> 
> 
> _______________________________________________
> tor-dev mailing list
> tor-dev at lists.torproject.org
> https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: OpenPGP digital signature
URL: <http://lists.torproject.org/pipermail/tor-dev/attachments/20130604/8f134a97/attachment.pgp>