crossbario / autobahn-cpp

WAMP for C++ in Boost/Asio
https://crossbar.io/autobahn
Boost Software License 1.0
251 stars 104 forks source link

wamp_session::process_challenge protocol_error throws will crash the application #232

Open pratman opened 2 years ago

pratman commented 2 years ago

m_session_join has to take the exception instead of throwing so connecting(and challenged) client can react on .get() instead of the whole application crashing from the unhandled exception (which is thrown inside of the asio_context loop).

one of the places with a possible fix: image

oberstet commented 2 years ago

why is that required? calling code can catch exceptions thrown in async code like this https://github.com/crossbario/autobahn-cpp/blob/master/examples/caller.cpp

oberstet commented 2 years ago

mmh ... wait .. maybe I'm confused .. other places in the session code do similar as you suggest:

https://github.com/crossbario/autobahn-cpp/blob/0fb8045bc57d5f80dc6f4781026f8422a31335d9/autobahn/wamp_session.ipp#L189

oberstet commented 2 years ago

aaah, ok: so the session code throws protocol errors, but uses setting the exception on a future in cases the application can / want to react to in a regular fashion.

so the actual Q is: in this specific case, the signature of the challenge sent by the client could not be verified by the router, and hence the router denies the client authentication.

this isn't a protocol error so ..

pratman commented 2 years ago

router didnt deny anything, send_message threw from any reason such as no transport. in any case application shouldnt crash. joining client is waiting on the .get() and should get the exception through the future (like everywhere else). only in this method is done incorrectly. i think real question is whether failing to send a message should crash an application

oberstet commented 2 years ago

well, why would throwing an exception crash a program necessarily? just catch it? not sure, I'm not using c++ often these days, and these arcane styles;)

anyways, this code has the pattern I believe to be correct:

https://github.com/crossbario/autobahn-cpp/blob/0fb8045bc57d5f80dc6f4781026f8422a31335d9/autobahn/wamp_session.ipp#L810

rethinking about the issue you posted: if a signature could not be verified, the WAMP openening handshake fails, and a completely new session and handshake must be performed (at the WAMP protocol level).

so that means, m_session_join should complete with error, but it is not a fatal error (like eg the router talking gibberish bytes ..)

pratman commented 2 years ago

Thanks for looking into this so quickly. This challenge handling happens within the asio context and exceptions thrown within these methods such as process_challenge cannot be caught by the application. The only way to signal such error from the context thread to the application is by using the futures, as done in the other methods. In process_abort as you mentioned above, the throws will also remain unhandled and lead to a crash.

Example of the stack:

VCRUNTIME140!_CxxThrowException+0x66 [throw.cpp @ 74] YOURAPP!::operator()+0x117 [wamp_session.ipp @ 788] YOURAPP!boost_asio_handler_invoke_helpers::invoke+0xe [handler_invoke_helpers.hpp @ 37] YOURAPP!boost::asio::detail::handler_work<,boost::asio::system_executor,boost::asio::system_executor>::complete+0xe [handler_work.hpp @ 100] YOURAPP!boost::asio::detail::completion_handler< >::do_complete+0x11d [completion_handler.hpp @ 70] YOURAPP!boost::asio::detail::win_iocp_operation::complete+0x1d [win_iocp_operation.hpp @ 47] YOURAPP!boost::asio::detail::win_iocp_io_context::do_one+0x356 [win_iocp_io_context.ipp @ 461] YOURAPP!boost::asio::detail::win_iocp_io_context::run+0xeb [win_iocp_io_context.ipp @ 203] YOURAPP!boost::asio::io_context::run+0x24 [io_context.ipp @ 64]