First two bytes incorrect for PUBLISH's with QoS 0 about mqtt-c HOT 5 CLOSED

_____ Broker: $ mosquitto -v -p 1884 1525878911: mosquitto version 1.4.8 (build date Thu, 01 Mar 2018 09:34:49 -0500) starting 1525878911: Using default config. 1525878911: Opening ipv4 listen socket on port 1884. 1525878911: Opening ipv6 listen socket on port 1884. 1525878935: New connection from 127.0.0.1 on port 1884. 1525878935: New client connected from 127.0.0.1 as publishing_client (c0, k400). 1525878935: Sending CONNACK to publishing_client (0, 0) 1525878948: New connection from 127.0.0.1 on port 1884. 1525878948: New client connected from 127.0.0.1 as subscribing_client (c0, k400). 1525878948: Sending CONNACK to subscribing_client (0, 0) 1525878948: Received SUBSCRIBE from subscribing_client 1525878948: datetime (QoS 0) 1525878948: subscribing_client 0 datetime 1525878948: Sending SUBACK to subscribing_client 1525878959: New connection from 127.0.0.1 on port 1884. 1525878959: New client connected from 127.0.0.1 as mosqsub/5264-workstatio (c1, k60). 1525878959: Sending CONNACK to mosqsub/5264-workstatio (0, 0) 1525878959: Received SUBSCRIBE from mosqsub/5264-workstatio 1525878959: datetime (QoS 0) 1525878959: mosqsub/5264-workstatio 0 datetime 1525878959: Sending SUBACK to mosqsub/5264-workstatio 1525878967: Received PUBLISH from publishing_client (d0, q0, r0, m0, 'datetime', ... (34 bytes)) 1525878967: Sending PUBLISH to subscribing_client (d0, q0, r0, m0, 'datetime', ... (34 bytes)) 1525878967: Sending PUBLISH to mosqsub/5264-workstatio (d0, q0, r0, m0, 'datetime', ... (34 bytes)) 1525878969: Received PUBLISH from publishing_client (d0, q0, r0, m0, 'datetime', ... (34 bytes)) 1525878969: Sending PUBLISH to subscribing_client (d0, q0, r0, m0, 'datetime', ... (34 bytes)) 1525878969: Sending PUBLISH to mosqsub/5264-workstatio (d0, q0, r0, m0, 'datetime', ... (34 bytes)) ^C1525878978: mosquitto version 1.4.8 terminating _____ Publisher (simple_publisher.c <https://github.com/LiamBindle/MQTT-C/blob/master/examples/simple_publisher.c> ): $ ./bin/simple_publisher 0.0.0.0 1884 ./bin/simple_publisher is ready to begin publishing the time. Press ENTER to publish the current time. Press CTRL-D (or any other key) to exit. ./bin/simple_publisher published : "The time is 2018-05-09 09:16:07" ./bin/simple_publisher published : "The time is 2018-05-09 09:16:09" ./bin/simple_publisher disconnecting from 0.0.0.0 _____ Mosquitto Subscriber: $ mosquitto_sub -h localhost -p 1884 -v -t datetime datetime �QThe time is 2018-05-09 09:16:07 datetime �(The time is 2018-05-09 09:16:09 _____ I found that I could reproduce these bad characters with simple_subscriber.c <https://github.com/LiamBindle/MQTT-C/blob/master/examples/simple_subscriber.c> by changing this line https://github.com/LiamBindle/MQTT-C/blob/5e18fed8c6e8f1015e809819da9a88dc77de52f0/examples/simple_subscriber.c#L123 to printf("Received publish('%s'): %s\n", topic_name, (const char*) published->application_message - 2); The resulting output was this: $ ./bin/simple_subscriber 0.0.0.0 1884 ./bin/simple_subscriber listening for 'datetime' messages. Press CTRL-D to exit. Received publish('datetime'): �QThe time is 2018-05-09 09:16:07 Received publish('datetime'): �(The time is 2018-05-09 09:16:09 _____ Diagnosing the bug One thing to notice is that the first character is always "�Q" and the second character is "�(". The last two characters of the variable header (two bytes preceding the payload) are the packet ID. MQTT-C uses an LFSR to generate packet ID's which would explain why the same sequence of bad characters shows up. It turns our packet ID's are only supposed to be included in the variable header for PUBLISH's with QOS > 0 (see mqtt-v3.1.1.pdf line 799-801). In mqtt_pack_publish_request packet ID's are being included in the variable header for all QoS levels (see snippet below). This means that this bug should only be present for PUBLISH's with QoS 0. https://github.com/LiamBindle/MQTT-C/blob/5e18fed8c6e8f1015e809819da9a88dc77de52f0/src/mqtt.c#L1124-L1131 How to fix This is actually an easy fix. We only want to include the variable header iff (publish_flags & 0x06) != 0 (i.e. QoS is not 0). This means that changing lines 1126-1127 to: 1126 if ((publish_flags & 0x06) != 0) { 1127 *(uint16_t*) buf = (uint16_t) MQTT_PAL_HTONS(packet_id); 1128 buf += 2; 1129 } Additionally, the PUBLISH response unpacker (mqtt_unpack_publish_response lines 1163-1164) needs to be updated to only unpack PID's if QoS > 0. 1163 if (response->qos_level > 0) { 1116 response->packet_id = (uint16_t) MQTT_PAL_NTOHS(*(uint16_t*) buf); 1165 buf += 2; 1166 } _____ Thanks for bringing this up! I will fix this tonight or tomorrow. Best regards, Liam — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/ACKouLVH3rmQFzv_vlPuQBP83U6cNBnXks5twxM4gaJpZM4T2sK2> . <https://github.com/notifications/beacon/ACKouAbBIWqSfZRIh81QBT9bGuLaDj7rks5twxM4gaJpZM4T2sK2.gif>