search

alexdalitz / dnsruby

Dnsruby is a feature-complete DNS(SEC) client for Ruby, as used by many of the world's largest DNS registries and the OpenDNSSEC project
Other
197 stars 77 forks source link

avoid changing global thread abort behavior #134

Closed busterb closed 7 years ago

busterb commented 7 years ago

It appears that a side-effect of requiring this library changes the global behavior of threads across the entire program. We noticed this while testing dnsruby in a Metasploit module. Is it sufficient just to remove this global change in behavior, or is this actually needed by dnsruby?

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.2%) to 76.609% when pulling b39649b6fd9989674bd8fbec1b81d6e19a12cd85 on busterb:global-flag-abort into 57648af1a77283b506b5b967025945597026b03b on alexdalitz:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.2%) to 76.609% when pulling b39649b6fd9989674bd8fbec1b81d6e19a12cd85 on busterb:global-flag-abort into 57648af1a77283b506b5b967025945597026b03b on alexdalitz:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.2%) to 76.609% when pulling b39649b6fd9989674bd8fbec1b81d6e19a12cd85 on busterb:global-flag-abort into 57648af1a77283b506b5b967025945597026b03b on alexdalitz:master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.003%) to 76.402% when pulling b39649b6fd9989674bd8fbec1b81d6e19a12cd85 on busterb:global-flag-abort into 57648af1a77283b506b5b967025945597026b03b on alexdalitz:master.

keithrbennett commented 7 years ago

I think I agree with @busterb. I was under the impression that setting Thread.abort_on_exception to true was a good idea, but that was because I didn't really understand it until it was explained to me by Will Murphy (https://willmurphyscode.net/) of my home area Ruby meetup in Arlington, Virginia, USA. Will pointed me to this StackOverflow page: https://stackoverflow.com/questions/9095316/handling-exceptions-raised-in-a-ruby-thread/9095369#9095369. Also, the Ruby documentation at https://ruby-doc.org/core-2.2.0/Thread.html#method-i-abort_on_exception was helpful.

Calling the class method with true will cause the Ruby runtime to exit if any threads raise an error. Calling the instance method isn't very helpful in our case either because we'd be calling it on our (dnsruby's) thread, which would result in the entire runtime exiting if any of the dnsruby threads raised an exception.

Clearly this is behavior that a gem should not impose on the code that uses it.

I did not look into why we need(ed) it, or thought we need it in the dnsruby gem. Will pointed out to me that if we just need to detect that an error occurred and get information about that error, that information will be provided to us when we call wait or value on that thread. If there are multiple threads and we want to know as soon as any one of them raises an error, then we can put together some kind of loop that calls alive? on each thread and calls wait or value if false is returned. (This polling approach is an ugly kludge, of course, but maybe the best option; we'd have to put a sleep in there so as not to hog the CPU.)

@alexdalitz, what do you think? I know much of this code was written ten years ago, do you remember why that's there?

alexdalitz commented 7 years ago

oops

busterb commented 7 years ago

Cheers!