-
Notifications
You must be signed in to change notification settings - Fork 9
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
replace telnetlib with pwnlib as SimpleSocket base class #115
replace telnetlib with pwnlib as SimpleSocket base class #115
Conversation
src/enochecker/utils.py
Outdated
if timeout is None: | ||
timeout = self.current_default_timeout | ||
|
||
return super().recvn(size, timeout=timeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readn
is also in in Tubes
as alias, so we could use this here, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true! I'll remove that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah but we probably want to keep the automated timeout handling (or is this somehow handled in tubes, anyway?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, via the 'timeout' keyword arg!
I actually noticed that every recv
function can be called via a corresponding read
, same goes for send
and write
. Here is the code in question:
for _name in list(locals()):
if 'recv' in _name:
_name2 = _name.replace('recv', 'read')
elif 'send' in _name:
_name2 = _name.replace('send', 'write')
else:
continue
locals()[_name2] = make_wrapper(locals()[_name], _name2)
The rest of our wrappers are still needed though because of our use of snake_case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was talking about self.current_default_timeout
if we want to support this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then should I revert that last commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may be cleaner to handle timeout ourselves, also that way, we could add an additional exception_message
to show in the scoreboard on timeout/eof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how would we enforce the timeout? on the worker-level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally we calculated remaining time for each request, based on the total available time. I guess this is no longer strictly needed as the engine checks timeouts, too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
related to #113 and #114