search

ronin-rb / ronin-support

A support library for Ronin. Like activesupport, but for hacking!
https://ronin-rb.dev
GNU Lesser General Public License v3.0
25 stars 9 forks source link

feat: connect methods' block behavior #505

Closed flavorjones closed 2 months ago

flavorjones commented 2 months ago

For the methods TCP.connect, UDP.connect, HTTP.connect, POP3::Mixin#pop3_connect, and SMTP::Mixin#smtp_connect:

flavorjones commented 2 months ago

do you think there might be value in leaving it open for debugging purposes, such as if you used ruby-debug or pry?

I don't personally see the value in leaving it open for potential debugging purposes, since a) developers can still set a breakpoint inside the block, and b) it risks a resource leak, especially if anybody is calling tcp_connect in a loop that's retrying (as in the case of a hard-to-reproduce exploit). I think the downside of having a lot of unclosed connections warrants this change.

Would you consider this more of a bug fix or an enhancement/feature

:thinking: Well, I think the "return the block's value" is a feature, but closing the connection is a bugfix. If your primary concern in asking is "should this go out in the imminent 1.1.0 release or can it wait for the next minor release" I think they can both probably wait.

We might also want to add this logic to the other lib/ronin/support/network/*/mixin.rb files

OK! I will add them to this PR today. Edit: Done!

postmodern commented 2 months ago

I guess we could argue that this is a bug fix since we are correcting the behavior to be the "correct" behavior. I can squash merge this and queue it up for the next patch release.