Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: connect methods' block behavior #505

Merged
merged 4 commits into from
May 6, 2024

Conversation

flavorjones
Copy link
Contributor

@flavorjones flavorjones commented May 4, 2024

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

  • return the block's return value
  • TCP, UDP, POP3, SMTP: close the socket even if the block raises an exception

@flavorjones flavorjones force-pushed the flavorjones-tcp-close-block branch from 0fc8bdb to fa0cf83 Compare May 4, 2024 17:18
@flavorjones flavorjones changed the title feat: TCP.connect and UDP.connect block behavior feat: TCP.connect, UDP.connect, and HTTP.connect block behavior May 4, 2024
- return the block's return value
- TCP/UDP: close the socket even if the block raises an exception
@flavorjones flavorjones force-pushed the flavorjones-tcp-close-block branch from fa0cf83 to 34fea0c Compare May 4, 2024 17:20
Copy link
Member

@postmodern postmodern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • While I generally agree that the connection should be closed on exception, do you think there might be value in leaving it open for debugging purposes, such as if you used ruby-debug or pry?
  • Would you consider this more of a bug fix or an enhancement/feature? If it's a change in behavior or new feature, then you should rebase against the 1.1.0 branch. If it's more of a bug fix, then branching off of main is fine.

Copy link
Member

@postmodern postmodern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might also want to add this logic to the other lib/ronin/support/network/*/mixin.rb files which define methods that will close a connection if a block is given:

  • lib/ronin/support/network/pop3/mixin.rb
  • lib/ronin/support/network/smtp/mixin.rb

@flavorjones
Copy link
Contributor Author

flavorjones commented May 5, 2024

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

🤔 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!

- return the block's return value
- close the socket even if the block raises an exception
@flavorjones flavorjones changed the title feat: TCP.connect, UDP.connect, and HTTP.connect block behavior feat: connect methods' block behavior May 5, 2024
@postmodern
Copy link
Member

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.

@postmodern postmodern merged commit b3a883e into ronin-rb:main May 6, 2024
5 checks passed
@postmodern postmodern added this to the 1.0.6 milestone May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http HTTP network Network tcp TCP udp UDP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants