X-Ryl669 / eMQTT5

An embedded MQTTv5 client in C++ with minimal footprint, maximal performance
MIT License
65 stars 14 forks source link

Adding properties to PUBLISH packet. #19

Closed turbosapiens closed 7 months ago

turbosapiens commented 7 months ago

Hi! I tried to pass some props to "publish" method.

...

Protocol::MQTT::V5::registerAllProperties();
Network::Client::MQTTv5::Properties properties;
if (!correlation_data.empty()) {
        Network::Client::MQTTv5::DynamicBinDataView c_data(correlation_data.length(), (const uint8_t*)correlation_data.c_str());
        Protocol::MQTT::V5::Property<Network::Client::MQTTv5::DynamicBinDataView> prop(Protocol::MQTT::V5::CorrelationData, c_data);
        if (!properties.append(&prop))
            printf("PROP APPEND FAILED!\n");
        else
            printf("PROP APPEND SUCCESS!\n");
}
client.publish(publish_topic, (const uint8*) publish_message,
                                            strlen(publish_message), retain_published_message, QoS, (uint16)0U, &properties))

But, i recieve

pure virtual method called
terminate called without an active exception

in

uint32 copyInto(uint8 * buffer) 
{
    o = 1; buffer[0] = header.typeAndFlags;
    o += remLength.copyInto(buffer+o);
    o += fixedVariableHeader.copyInto(buffer+o);
    o += props.copyInto(buffer+o);  //HERE
    o += payload.copyInto(buffer+o);
    return o;
}

Maybe i'm just doing something really stupid, but i can't make it work. But if i just create empty Network::Client::MQTTv5::Properties properties; it works (without any property ofc)

turbosapiens commented 7 months ago

I also tried your example like that

 // Please do not move the line below as it must outlive the packet
        Protocol::MQTT::V5::Property<uint32> maxProp(Protocol::MQTT::V5::PacketSizeMax, 2048);
        Protocol::MQTT::V5::Property<Protocol::MQTT::Common::DynamicStringPair> userProp(Protocol::MQTT::V5::UserProperty, Protocol::MQTT::Common::DynamicStringPair("key", "value"));
        //Protocol::MQTT::V5::ControlPacket<Protocol::MQTT::V5::CONNECT> packet;

        // Check if we have a max packet size property and if not, append one to let the server know our limitation (if any)
        //packet.props.append(&maxProp); // It'll fail silently if it already exists
        props.append(&userProp); // It'll fail silently if it already exists

Same behaviour

X-Ryl669 commented 7 months ago

For your code, the issue is that you're making a pointer that's pointing to an object that's deleted. The prop instance in your if block doesn't survive after the if block. So the Properties object in publish in now referencing a dangling object.

You have two solutions to your issue.

Solution 1: Still use the property that's allocated on the stack, you'll need to move the declaration in the same scope as the usage, like this:

Protocol::MQTT::V5::registerAllProperties();
Network::Client::MQTTv5::Properties properties;
Network::Client::MQTTv5::DynamicBinDataView c_data(correlation_data.length(), (const uint8_t*)correlation_data.c_str());
Protocol::MQTT::V5::Property<Network::Client::MQTTv5::DynamicBinDataView> prop(Protocol::MQTT::V5::CorrelationData, c_data);
if (!correlation_data.empty()) {
       if (!properties.append(&prop))
           printf("PROP APPEND FAILED!\n");
       else
           printf("PROP APPEND SUCCESS!\n");
}
client.publish(publish_topic, (const uint8*) publish_message,
                                           strlen(publish_message), retain_published_message, QoS, (uint16)0U, &properties))

This is valid because std::string::c_str returns a valid zero terminated string if empty (see here). It would be also valid if it returned nullptr since you're using a view and a view can be null. The property prop will be destructed at end of the function scope, so after publish used it and no pointer is dangling in that case.

Solution 2: Use a property that's allocated on the heap instead Write like this:

Protocol::MQTT::V5::registerAllProperties();
Network::Client::MQTTv5::Properties properties;
if (!correlation_data.empty()) {
       Network::Client::MQTTv5::DynamicBinDataView c_data(correlation_data.length(), (const uint8_t*)correlation_data.c_str());
       auto * prop = new Protocol::MQTT::V5::Property<Network::Client::MQTTv5::DynamicBinDataView> (Protocol::MQTT::V5::CorrelationData, c_data, true);
       if (!properties.append(prop))
           printf("PROP APPEND FAILED!\n");
       else
           printf("PROP APPEND SUCCESS!\n");
}
client.publish(publish_topic, (const uint8*) publish_message,
                                           strlen(publish_message), retain_published_message, QoS, (uint16)0U, &properties))

Notice the , true in the end of the Property constructor that tells the library to delete the instance when the Properties is being destructed.

Hope that helps!

turbosapiens commented 7 months ago

Sorry for the inconvenience, i'm idiot Completely forgot about scope and if

X-Ryl669 commented 7 months ago

That's ok, the interface in eMQTT5 is designed for embedded development, so when a pointer is used in the methods signature, it often doesn't expect a heap allocated object (or you have to explicitly tell it that it's a pointer to the heap). Dealing with pointers on the stack that self destruct is not usual for a C++ developer, I can easily understand it's disturbing.