Invalid command payload data crashes ozwdaemon

[kpine]

I used payload data that was in an unexpected format and it crashed.

[20200507 10:38:42.410 PDT] [qt.mqtt.connection.verbose] [debug]: Finalize PUBLISH: topic: QMqttTopicName("OpenZWave/1/command/hasnodefailed/")  payloadLength: 18
[20200507 10:38:42.410 PDT] [ozw.mqtt.commands] [debug]: Got  "OpenZWave/1/command/hasnodefailed/"  Message:  "\"asdfasdfsadfasdf\""
ozwdaemon: /usr/include/rapidjson/document.h:1078: rapidjson::GenericValue<Encoding, Allocator>::ConstMemberIterator rapidjson::GenericValue<Encoding, Allocator>::MemberEnd() const [with Encoding = rapidjson::UTF8<>; Allocator = rapidjson::MemoryPoolAllocator<>; rapidjson::GenericValue<Encoding, Allocator>:
:ConstMemberIterator = rapidjson::GenericMemberIterator<true, rapidjson::UTF8<>, rapidjson::MemoryPoolAllocator<> >]: Assertion `IsObject()' failed.
[20200507 10:38:46.316 PDT] [default] [warning]: =============================
[20200507 10:38:46.316 PDT] [default] [warning]: CRASH!!! - Dumping Backtrace:
[20200507 10:38:46.316 PDT] [default] [warning]: =============================
[20200507 10:38:46.326 PDT] [default] [warning]: #1  0x0000557fd493e3d6 sp=0x0000557fd5ded260 dumpCallback(google_breakpad::MinidumpDescriptor const&, void*, bool) + 0x36
[20200507 10:38:46.326 PDT] [default] [warning]: #2  0x0000557fd4956f25 sp=0x0000557fd5ded4d0 google_breakpad::ExceptionHandler::GenerateDump(google_breakpad::ExceptionHandler::CrashContext*) + 0x385
[20200507 10:38:46.327 PDT] [default] [warning]: #3  0x0000557fd4957288 sp=0x0000557fd5ded570 google_breakpad::ExceptionHandler::SignalHandler(int, siginfo_t*, void*) + 0xa8
[20200507 10:38:46.329 PDT] [default] [warning]: #4  0x00007fae2f0ac110 sp=0x0000557fd5ded640 funlockfile + 0x50
[20200507 10:38:46.330 PDT] [default] [warning]: #5  0x00007fae2ec2d761 sp=0x00007ffdf282b120 gsignal + 0x141
[20200507 10:38:46.331 PDT] [default] [warning]: #6  0x00007fae2ec1755b sp=0x00007ffdf282b240 abort + 0x127
[20200507 10:38:46.332 PDT] [default] [warning]: #7  0x00007fae2ec1742f sp=0x00007ffdf282b370 __libc_freeres + 0x127
[20200507 10:38:46.333 PDT] [default] [warning]: #8  0x00007fae2ec26092 sp=0x00007ffdf282b3c0 __assert_fail + 0x42
[20200507 10:38:46.333 PDT] [default] [warning]: #9  0x0000557fd492b7f2 sp=0x00007ffdf282b3f0 MqttCommand::messageReceived(QMqttMessage) + 0x25e2
[20200507 10:38:46.333 PDT] [default] [warning]: #10 0x0000557fd492bf1b sp=0x00007ffdf282b680 QtPrivate::QSlotObject<void (MqttCommand::*)(QMqttMessage), QtPrivate::List<QMqttMessage>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) + 0x4b
[20200507 10:38:46.335 PDT] [default] [warning]: #11 0x00007fae2f3624e8 sp=0x00007ffdf282b6b0 QMetaObject::activate(QObject*, int, int, void**) + 0x918
[20200507 10:38:46.339 PDT] [default] [warning]: #12 0x00007fae2f7d1c75 sp=0x00007ffdf282b7a0 QMqttSubscription::messageReceived(QMqttMessage) + 0x35
[20200507 10:38:46.340 PDT] [default] [warning]: #13 0x00007fae2f7c78ea sp=0x00007ffdf282b7d0 QMqttConnection::finalize_publish() + 0x20a
[20200507 10:38:46.341 PDT] [default] [warning]: #14 0x00007fae2f7c8435 sp=0x00007ffdf282b8a0 QMqttConnection::processDataHelper() + 0x5b5
[20200507 10:38:46.343 PDT] [default] [warning]: #15 0x00007fae2f7c88a0 sp=0x00007ffdf282b900 QMqttConnection::transportReadReady() + 0x70
[20200507 10:38:46.344 PDT] [default] [warning]: #16 0x00007fae2f3624e8 sp=0x00007ffdf282b960 QMetaObject::activate(QObject*, int, int, void**) + 0x918
[20200507 10:38:46.345 PDT] [default] [warning]: #17 0x00007fae2f6dc874 sp=0x00007ffdf282ba50 QAbstractSocket::setSocketDescriptor(long long, QAbstractSocket::SocketState, QFlags<QIODevice::OpenModeFlag>) + 0x664
[20200507 10:38:46.348 PDT] [default] [warning]: #18 0x00007fae2f6dc90b sp=0x00007ffdf282ba70 QAbstractSocket::setSocketDescriptor(long long, QAbstractSocket::SocketState, QFlags<QIODevice::OpenModeFlag>) + 0x6fb
[20200507 10:38:46.349 PDT] [default] [warning]: #19 0x00007fae2f6ee921 sp=0x00007ffdf282baa0 QTcpServer::incomingConnection(long long) + 0xb561
[20200507 10:38:46.350 PDT] [default] [warning]: #20 0x00007fae2f337a2f sp=0x00007ffdf282bab0 QCoreApplication::notifyInternal2(QObject*, QEvent*) + 0x15f
[20200507 10:38:46.351 PDT] [default] [warning]: #21 0x00007fae2f38e3c5 sp=0x00007ffdf282bb20 QEventDispatcherGlib::registerSocketNotifier(QSocketNotifier*) + 0x145
[20200507 10:38:46.352 PDT] [default] [warning]: #22 0x00007fae2db9f60d sp=0x00007ffdf282bb60 g_main_context_dispatch + 0x27d
[20200507 10:38:46.354 PDT] [default] [warning]: #23 0x00007fae2db9f890 sp=0x00007ffdf282bbe0 g_main_context_dispatch + 0x500
[20200507 10:38:46.355 PDT] [default] [warning]: #24 0x00007fae2db9f91f sp=0x00007ffdf282bc40 g_main_context_iteration + 0x2f
[20200507 10:38:46.356 PDT] [default] [warning]: #25 0x00007fae2f38d7c1 sp=0x00007ffdf282bc60 QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) + 0x61
[20200507 10:38:46.357 PDT] [default] [warning]: #26 0x00007fae2f3366db sp=0x00007ffdf282bcb0 QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) + 0x12b
[20200507 10:38:46.359 PDT] [default] [warning]: #27 0x00007fae2f33e182 sp=0x00007ffdf282bd30 QCoreApplication::exec() + 0x92
[20200507 10:38:46.359 PDT] [default] [warning]: #28 0x0000557fd48fd751 sp=0x00007ffdf282bd90 main + 0x2ad1
[20200507 10:38:46.360 PDT] [default] [warning]: #29 0x00007fae2ec18e0b sp=0x00007ffdf282c2d0 __libc_start_main + 0xeb
[20200507 10:38:46.361 PDT] [default] [warning]: #30 0x0000557fd48fe9da sp=0x00007ffdf282c390 _start + 0x2a
[20200507 10:38:46.361 PDT] [default] [warning]: dumpCallback Succeeded:  true  at  /opt/ozw/config/crashes//38949278-3601-46b2-7cda9392-a645427d.dmp
[20200507 10:38:46.361 PDT] [default] [warning]: Uploading MiniDump to  https://sentry.io/api/1868130/minidump/?sentry_key=e086ba93030843199aab391947d205da
[20200507 10:38:46.743 PDT] [default] [warning]: Uploaded Crash minidump With ID:  f67af4c9-683b-4b8b-8b28-6faf12f36e81

[kpine]

Follow up question, is it intentional that commands without parameters require a payload? I'm no MQTT expert, but from my understanding payloads are not required. I was assuming that commands with no data did not need a payload. For example, this won't work:

OpenZWave/1/command/removenode/

But these work equally well:

OpenZWave/1/command/removenode/
""

OpenZWave/1/command/removenode/
"asdfasdf"

OpenZWave/1/command/removenode/
"{}"

Basically, as long as a command has any payload, it will be accepted by ozwdaemon. I produced the crash above because I sent the wrong data to a command that expected a JSON object instead. If a payload was required, would it make more sense to force it to be something consistent like an empty JSON object, or is it fine the way it is?

BTW, the HA code is sending the empty string "", which is how I was able to determine this. I was trying to issue the removenode command manually via MQTT and it was not working, then noticed HA sends all commands with payload "" when no parameters are set.

[Fishwaldo]

Fixed the crash - All Commands will need a "Payload" at least to pass - which brings me to:

is it intentional that commands without parameters require a payload?

I'm also not a MQTT export - but based upon the API used to talk to MQTT - The way we "remove" retained topics is to send a empty payload - So I'm assuming we should send something (a empty JSON object now for example).

Secondly - The crash was actually happening in the logic that checks if we have a valid parameters for each command. There are a few commands (cancelcontrollercommand for example) that don't accept any parameters - This change means we only accept a empty JSON object "{ }" now, not a empty string. (otherwise the logic around that becomes a lot more complex!)

@marcelveldt For your info - ozwdaemon will error out if your sending empty strings as @kpine pointed out now.

[kpine]

No arguments from me, I'll just add a few notes.

I'm also not a MQTT export - but based upon the API used to talk to MQTT - The way we "remove" retained topics is to send a empty payload - So I'm assuming we should send something (a empty JSON object now for example).

That sounds about right... I think. The spec says:

It is valid for a PUBLISH Packet to contain a zero length payload.

For messages with the retain flag an empty payload has special meaning:

PUBLISH Packet with a RETAIN flag set to 1 and a payload containing zero bytes will be processed as normal by the Server and sent to Clients with a subscription matching the topic name. Additionally any existing retained message with the same topic name MUST be removed and any future subscribers for the topic will not receive a retained message [MQTT-3.3.1-10]. “As normal” means that the RETAIN flag is not set in the message received by existing Clients. A zero byte retained message MUST NOT be stored as a retained message on the Server [MQTT-3.3.1-11].

Whereas if the retain flag is false:

If the RETAIN flag is 0, in a PUBLISH Packet sent by a Client to a Server, the Server MUST NOT store the message and MUST NOT remove or replace any existing retained message [MQTT-3.3.1-12].

So, you would probably be fine allowing the commands to have no payload, but it makes sense to make the API uniform.

[marcelveldt]

Thanks for the headsup. We'll adjust accordingly.

[kpine]

Hi @marcelveldt, do you have plans to address this one soon, or would you like an issue submitted somewhere?