String payload when using publish() is sent with terminating /0

[Pablo2048]

It seems like in V0.0.8 the String() (and probably std::string) payload is sent with terminating /0 character when using publish(). The problem seems to be in payload.size()+1 introduced here https://github.com/philbowles/PangolinMQTT/blob/300d2b38689f66db6344da2488ea429116555ef5/src/PangolinMQTT.h#L217

[philbowles]

It is, and it is correct. It has two functions:

1) It is so that PANGO::payloadToString etc routines don't run off the end of the buffer. Thus it is stored as a true C-string for easy retrieval using PANGO:: utilities. There is no reason to try to retrieve it manually using actual data length - that is the point of the utility routines, to make life easier for you.

2) Its absence was one of the other massive bugs in AsyncMqttClient - the author (and I guess many users) just assumed that the payload was \0 terminated. It is not, and never has been: it is length-counted. Luckily for everyone, purely by chance in 99% of cases there did just happen to be a \0 at end_buffer+1 (probably the high byte of a value less than 2^16). Of course in the cases where there coincidentally wasn't you'd get a crash.

SO this is also a "safety mechanism" for people who don't understand that, so that if they pass the buffer pointer to anything expecting a C-String, they now genuinely get one instead of accidentally getting lucky as they did before.

You can safely ignore it. Hope this satisfies you!

[Pablo2048]

Thank You for explanation. This behavior should be mentioned somewhere in documentation, because it break backward compatibility with AsyncMQTT and made use String payloads actually unuseable with some third-party MQTT clients. For example http://mqtt-explorer.com/ is unable to decode JSON message with terminating 0.

[philbowles]

Then it is up to the user to decode the payload according to its content. It is also up to the user to set the payload correctly. If you don't want a trailing \0 then use the correct function call of uint8_t* / length. If you use the "abbreviated" type overloads that calculate the length for you, you will get the trailing \0 so the the type-specific decodes function correctly. If that's not what you want / need: don't call them. They are there so that you get a valid result without having to know the length.

So having the correct type / length functions available PLUS extra ease-of-use functions does not / cannot "break" anything. Backwards compatibility with AsyncMqttClient is something I am absolutely keen to avoid since that API was full of errors, and illogicalities and - in my opinion - showed that the author didnt really have a clue what he was doing, so anything that moves away from that is a good thing. I saw more "schoolboy errors" and terrible coding and fundamental misunderstandings of basic type handling in that lib than the whole for the last 10 years, so replicating it exactly is never going to be on my todo list.

Part of the problem withi his API was exactly this: he didn't seem to know the difference between a C-string and String and a std::string, nor that a length-counted block of arbitrary data is NOT any of the above.

As I mentioned before, if you use the PANGO:: utilities to retrieve your JSON string then it will not be \0 terminated when you then pass it to a JSON decoder - provided that you put it in there with a type overload length-calculating function unless I'm missing something.

If you decide to "Unpack" the packet yourself and construct a String from it using the payload length, then I cannot be accountable for how you do that...all you are doing is reinventing the wheel of the PANGO:: utilities.

To be honest, I think the docs are quite clear about this: the fact that a lot of people have been doing it wrong for a long time is no reason for me to make my code match other people's incorrect assumptions. The contents of the payload are 100% user responsibility.

If third-party code puts correct data / length into a payload, then it is up to the user to decode it appropriately given the senders intention / specification. I'm guessing 99% of apps will do it correctly. If there is a body of code out there that copied the incorrect, illogical and ignorant methods they learned from a very poorly written library, then "breaking backwards compatibility" with that is something I not only promote, but take pride in.

Don't mix the methods: EITHER store a uint8_t + its exact length and then unpack it yourself using payload and length....OR store with type overload and retrieve with PANGO:: utility function and forget about lengths entirely.

If there is genuinley a bug in the code behind that above statement, of course I will fix it, but I can't fix users' bad habits (not saying tht applies to you :), I mean generally ) . At the moment I'm not convinced.

[philbowles]

PS It might help if we had a code snippet to discuss: I can then suggest the best method(s)

[Pablo2048]

I see your point and I don't have problem using uint8_t* (even when You marked it as stupid!!! :) ), but there probably won't be just communication PANGO<->broker<->PANGO, which solve this. There is reason in MQTT standard for using length instead of C string IMO - there is no standard message format/type identifier in MQTT message so UTF-8 encoded string will probably not pass http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/cos02/mqtt-v3.1.1-cos02.html . Actually I just change my original code from AsyncMqtt

mqttClient->publish(mqttLastWill, 1, true, lwOk);

to PangolinMQTT v0.0.7

mqttClient->publish(mqttLastWill, 1, true, String(lwOk));

and now to this (v0.0.8)

mqttClient->publish(mqttLastWill, 1, true, (uint8_t *)&lwOk[0], strlen(lwOk), false);

I personally don't like using the String class, but I did it for speed-up testing, so I'm happy to switch back to the pointers. I just want to report it to inform You and probably others.

[philbowles]

Yes, the only thing MQTT say about the payload is it is depedent upon the application, which measn it can hold anything. We shouldn't get "hung up" on string data - it could just as easily be a chunk of encyrpted fimware - and that's the point: its is never a string,,,unless the user chooses to make it one.

I don't understand your point "UTF-8 encoded string will probably not pass"

You need to read the documenation more carefully: I don't call uint8_t "stupid" - I insist several times its the only and correct defintion. What I called "stupid" in the comments was the existing API having the dup flag. That's one of the pointers to the "other author" having no clue about what he was doing. The user should NEVER be allowed to touch that flag. The only time it is set in an outgoing packet is when the lib enters session recovery mode to re-transmit previoulsy failed QoS1 messages.

Setting it at any other time in any other message will break QoS1 or cause the server to abort the connection. BUT since "the other library"didnt even attempt session recovery, did not correctly implement QoS1 at all and had no goddamn idea about resending anything (it could even send its own data properly the first time in many cases) - then it is no surpise to find such glaring stupidities and demonstrations of ignorance in its API.

"Stupid" was being polite. I wouldnt trust its author to write "Blinky" without getting at least 3 or 4 basic errors into it and having it randomly crash. I have never so much BS and ignornace in one place since Donald Trump last spoke.

[obrain17]

Outside of all discussions about some old libraries I think that in real life most payloads are arrays of readable characters - but without ending /0. Every decent MQTT client will understand and correctly handle these using the "len"-parameter.

So I am using either the full syntax with uint8_t payload and length or if I am sure I really pass a C-string (with ending \0 !) I have defined an own function:

void MqttPublishCString(const char* topic, uint8_t qos, bool retain, const char *payload){_mqttClient.publish(topic,qos,retain, (uint8_t*) payload, (size_t) strlen(payload),false); }

So I have the option to benefit from the new library while not having to change all my existing code ...

[philbowles]

"I think that in real life most payloads are arrays of readable characters" possibly true - but that makes no difference to the fact that an MQTT payload is not "an array of readable characters", unless the user choose to put an array of readable characters in it!!!. Any full implementation must adhere to the spec, the rules etc. which Pangolin does - to the letter.

Did you ever think that the reason it may be true is because there are so many implementations that incorrectly allow nothing else or require additional code by the user to do so properly? Pangolin implements the MQTT payload specifiction 100% correctly and provides easy one-liners for quick / lazy / unaware folk to "just" pub and sub strings. It is the best of both worlds - why are we still even discussing it? If people/other libraries have got into the habit of not doing that, then it is not a Pangolin issue!

I say AGAIN: MQTT payloads are length-counted blocks of arbitrary bytes. They are only "strings of..." or "B-TREE node lists..." or "JSON X..." or "XML-wrapped binary cryptos...." if that is what the user puts in them

A room with an elephant in it, is not an elephant! A payload is a container, nothing more. It is whatver you want it to be and everybody wants different stuff. MQTT allows any stuff and so Pangolin must do so too.

IF you do not attach a \0 to your readable string then when whoever receives it MUST unpack it by length and build a string...with a \0 attached. Pangolin saves you those two steps - why are we still discussing it? Why are we worrying about the correct internal structure of payload when Pangolin provides functions at application level to "just put in a string" and "just pull out a string". One line of code in, one line out. - what could be simpler?

I do wish that people would understand that any other implementation requires lots of extra code for the user, with buffer allocations that can be forgetton to free etc etc etc.

I do wish also that they would understand that they are wrong when they talk about "strings" in payloads without a trailing \0.

A series of characters which is not terminated by a \0 is not a "string". !!! So if you want to send a string == "XXXX" then that is stored internally as FIVE bytes. So if you put in only 4x 'X' and set the payload length to 4 you have not sent a string you have sent four 'X's. At the other end you need to reserve FIVE bytes, copy the 4x 'X's set the fifth byte to \0 and only then do you have a string. And why bother when Pangolin does exactly that for you? It the correct way to send a string. It is the only way to send a "string" - anything else is a count-defined block of bytes not a string.

So, good people, please accept that you, and everybody else who thinks they have been sending strings is wrong and that Pangolin is correct. The entire population of the earth used to think the sun revolved around it till Galileo proved all of them wrong. Just enjoy the one-line-in / one-line out functions, forget about the correctly added NULs and get on with your apps!

And thank you for your support - it is much appreciated. :)

[Pablo2048]

Ok, even when I really appreciate the work which Phil does, I have to write this as my last contribution here because I don't want to waste my time in the direction that this discussion heads.

MQTT payload, which contain series of characters IS string by definition, period. I wouldn't discuss about this anymore. Series of characters with \0 as an terminator is INTERNAL representation, called ASCIIZ, C-string, ... which is SPECIFIC for used programming language. Another internal representation can be Pascal-like string, which has it's length present in front - BTW even MQTT standard uses Pascal-like format for storing strings. If you are using language which expect any form of terminator you have to add it by yourself, but do not cripple the message that others can see!

If I use your elephant example: Pangolin client publish an elephant, another Pangolin client receive an elephant, but every other client in the world sees something, which looks like an elephant with an knot on his tail just because Pangolin need to mark "this is the end of the elephant" with the knot. It's not just bad, it's ridiculous. MQTT was designed to be agnostic enough so clients from many computer languages can communicate together, and it lacks of payload type specification (we are talking about 3.1.1, in 5.0 there is Payload Format Indicator, which value = 1 specifies UTF-8 encoded payload). No one knows about client/language they are communicating with so why I do bother to add something to the payload just because someone THINK that his programming language specific format is the only right, but is even unable to tell the opposite side about his thinking?

Even worse are another consequences using this "feature" - you have to get in mind that EVERY tool/application you used or want to communicate with can handle it "properly" and don't forget to add the terminator manually when publishing topics by hand. So let's see what happen when somebody, which don't know that he's communicating with Pangolin send his string payload without the terminator - the payload arrive and Pangolin uses his PANGO::payloadToCstring() method. Then it crashes because payloadToCstring allocate memory buffer for string AND EXPECT THAT THE /0 IS INSIDE PAYLOAD, which is not so it create Cstring without an terminator. It is safer? Hell, no. All this can be solved very easily - get rid of the (artificial) terminator when publishing and put it back when reading as an string. There exist examples for even Mosquitto or Paho doing exactly this so there is no need to reinvent the wheel.

This is WHY I want to discuss this.

I don't care anymore if this would be taken in mind or not, I'm writting this here just in case that someone use Pangolin and somewhere on other side there is my application (and probably not just mine) which use MQTT.js, or Paho, or any other MQTT client library and he got into trouble because of this silly feature so I can show why it is happen. No, I'm not changing my own code just to get rid of terminator on receiving and adding it back when publishing. MQTT JSON payload with added /0 character is malformed one and will be rejected. Period.

[leifclaesson]

Phil, GREAT work on this library! AsyncMQTTClient has caused no end of trouble for me. Being TCP-based, I figured "what's there to screw up" so I never looked more closely, just worked around the problems with ever-more elaborate retry-schemes. Yuck! Looking forward to finally getting to clean up my code.

I also could not agree more with you about the sorry waste of oxygen currently occupying the white house.

However, when it comes to null-terminated strings inside MQTT payloads, I guarantee this is going remain a point of contention. You are absolutely right that the MQTT payloads are binary blobs.. but they do have a defined length. They don't need a terminating null character.. and in fact, MQTT explorer even displays the null character as an ugly box. The spec does not call for a null character. Other MQTT apps do not add one. MQTT doesn't care that we're using C. The onus is really on the application dealing with the data to add a terminating null character at the right place, if it needs it to deal with strings.

When I wrote my own LeifHomieLib (based on AsyncMqttClient, god help me), at first I published with the null character inside the data, but once I noticed that it showed up in MQTT Explorer, I realized that this was not how it was supposed to be done. So, made my code publish with the length field set correctly, and when I get an MQTT message, I simply assign() it to an std::string with the right length!

Reading some of your comments, I remember having a similar mindset when I was in my 20s. I knew I was right, nobody was going to tell me otherwise. Now that I'm in my 40s, I've gotten a lot better at adapting, and in the case of MQTT, there is already an eco-system of existing applications to adapt to, with defined-length strings seemingly the most common way of presenting text data through MQTT.

It's great that you're adding a null-character, but it's in the wrong place. Judging from the huge installed base, it would be much more helpful if the null character was added in a payloadToCString function.

I can do without it, and obviously I'll publish with the uint8_t * function, but I know I won't be the last person to get caught in this gotcha. I certainly wasn't the first.

By the way, couldn't uint8_t payload be const uint8_t ?

Thanks again for a much, much better library than I've previously had the misfortune of using. I read through your list of bugs and couldn't believe my eyes.. it explains so much. That said, a human being wrote the library, and his heart was in the right place releasing the library freely (and I'm sure he didn't mean for it to suck) so perhaps we shouldn't judge him too harshly.. we all have to start somewhere.

Peace! ///Leif

[pfeerick]

I 100% agree... there has been some excellent work done on this library - my limited testing of it so far shows it to be stable, and not introducing any crashes into somewhat already busy async wifi + http code. However, keeping the null terminator of String variables is a no-go (although it's technically not keeping the null terminator, but (size_t) payload.size()+1 setting the payload one too many)... can't just do a String(intVar) to cast a number to a string for publishing without it being mangled by PangoMQTT.

What stopped you from adding extra function declarations, like the following, allowing the length of std::string and Strings to be defined when publishing?

void publish(const char* topic, uint8_t qos, bool retain, std::string payload, size_t length){ publish(topic,qos,retain, (uint8_t*) payload.data()length,false); }
void publish(const char* topic, uint8_t qos, bool retain, String payload, size_t length){ publish(topic,qos,retain, (uint8_t*) payload.c_str(),length,false); }

This would have stopped this n+1 nonsense, as the length is easily determined with strlen or .lenth().

i.e. instead of

it could be

[CODeRUS]

This is ridiculous. Please fix.