send_connack ends in assertion.

[Teuling]

I'm connecting to mqtt_cpp from an (older) mosquitto client.

The code ends up in an assertion when calling connack on the endpoint (unrecognized protocol version).

Shouldn't the stack be robust to this, even if the mosquitto client somehow sets something inappropriate for the protocol version?

https://github.com/redboltz/mqtt_cpp/blob/3b1dfe2392a3eaad94ca6520d1746323fdf4ee68/include/mqtt/endpoint.hpp#L9625

[redboltz]

Thank you for reporting the issue. I need more information. Could you post the backtrace of assertion failed case? I need to know who call it.

Possible issue:

1. BOOST_ASSERT(false); could be too much. Throwing exception or output log and ignore it.
2. Caller's code could be invalid. Caller should avoid send_connack() with unrecognized protocol version. If the caller is broker.hpp, then broker.hpp should be fixed.
3. on_connect() could be invalid. If client send CONNECT with unsupported version ( only 4 and 5 are supported), on_connect() might not be called.

In order to analyze the root issue, I need more information.

Also I want to know the CONNECT packet's protocol version sent by old mosquitto client.

[redboltz]

I've checked a little.

On process_connack function, the received CONNECT packet's protocol_version is checked. If it is unsupported version, then call on_error() instead of on_connect(). See: https://github.com/redboltz/mqtt_cpp/blob/3b1dfe2392a3eaad94ca6520d1746323fdf4ee68/include/mqtt/endpoint.hpp#L7240-L7243

send_connack() should be called after on_connect() is handled. If user call send_connack() before on_connect() is handled, assertion fail is appropriate behavior.

If you are developing your custom mqtt server/broker using mqtt_cpp, and you call send_connack() as above, you should fix your code.

---

I'm still not sure what happend on your case. Anyway, I will wait your response. Thanks.

[redboltz]

I found invalid async_send_connack() call in broker.hpp. I will fix it.

https://github.com/redboltz/mqtt_cpp/blob/3b1dfe2392a3eaad94ca6520d1746323fdf4ee68/include/mqtt/broker/broker.hpp#L163-L192

It is weied that the issue is async_send_connack() call but not send_connack() call.

[redboltz]

I noticed that the fix isn't needed:

The protocol_version is checked correctly.

https://github.com/redboltz/mqtt_cpp/blob/3b1dfe2392a3eaad94ca6520d1746323fdf4ee68/include/mqtt/broker/broker.hpp#L196-L221

[Teuling]

Thank you for the thorough investigation, much appreciated.

The connack is send from the on_connect() handler.

I can't share the stack with you because of company confidentiality.

I'm still debugging the issue but it might be stack corruption.

I'll let you know if it is anything you have to deal with.

[Teuling]

I can confirm that the root cause is memory corruption that has nothing to do with the mqtt cpp stack.

You can close the issue.

Great work by the way and have a nice weekend.