MQTT Client: Added support for receiving empty payload (IDFGH-946)

[gregjesl]

Closes https://github.com/espressif/esp-idf/issues/3088

[gregjesl]

@david-cermak I've made the changes, please let me know if there is anything else.

[david-cermak]

@gregjesl LGTM, thanks.

However Travis has failed, will have to fix it and then ask for a rebase from your side.

[david-cermak]

@gregjesl Could you please rebase now? Build would probably still fail as it's also validated against IDF master (which might not currently build on some platforms: linux with newer python packages), but there's not an issue in this repo and changes can be reviewed internally in the meantime.

[gregjesl]

@david-cermak You'd like me to rebase the idf branch onto this pull request, correct?

[david-cermak]

@gregjesl Yes, please rebase your branch (in this pull request) on top of idf. That's then easier for us to test and review, plus don't have to modify your commits

[gregjesl]

@david-cermak I've rebased the branch.

[david-cermak]

@gregjesl There seems to be an issue with receiving empty messages under some conditions (e.g. single message with empty payload in a buffer).

Sorry I haven't noticed during code review and haven't had time to test it before. Conditional NULL == (mqtt_data = mqtt_get_publish_data(message, &mqtt_data_length) seems to be a culprit as this could be null for empty messages.

[gregjesl]

@david-cermak From the MQTT 3.1.1 spec:

The length of the payload can be calculated by subtracting the length of the variable header from the Remaining Length field that is in the Fixed Header.

From what I can tell the proper solution is to check for the existence of a payload by subtracting the Remaining Length field of the Fixed Header from the length of the variable header. If this value is greater than 0, then NULL == (mqtt_data = mqtt_get_publish_data(message, &mqtt_data_length) must be true for the full message to be received. If this value value is 0, then the full message has been received.

The length of the variable header is simply the topic length + 4. I can't find the part of the code that returns the Remaining Length field.

[david-cermak]

Generally, I agree that would be the correct solution. There's no API which directly returns "Remaining Length" (also note the variable header length also depends on QOS).

With the given codebase however, I might prefer to modify mqtt_get_publish_data (and maybe the topic function) to indicate if a starting payload ptr could be resolved. Something like:

bool mqtt_get_publish_data(uint8_t* buffer, uint32_t* length, char** mqtt_data);

[gregjesl]

@david-cermak The following function should return true if buffer contains a complete message:

bool mqtt_message_complete(uint8_t* buffer, uint32_t blength)
{
    int i;
    int remainlen = 0;

    // Read out the Remaining Length (which does not include the length of the fixed header)
    for (i = 1; i < 5; ++i)
    {
        if(i >= blength)
            return false;
        remainlen += (buffer[i] & 0x7f) << (7 * (i - 1));
        if ((buffer[i] & 0x80) == 0)
        {
            ++i;
            break;
        }
    }

    // The buffer length must be at least the Remaining Length 
    // plus the length of the fixed header (i in this case)
    return blength >= remainlen + i;
}

The test

if(NULL == (mqtt_topic = mqtt_get_publish_topic(message, &mqtt_topic_length)) || 
    NULL == (mqtt_data = mqtt_get_publish_data(message, &mqtt_data_length)))

can be replaced with

if(!mqtt_message_complete(message, length))

The buffer contains a complete message without a payload if

mqtt_message_complete(message, length) == true

and

NULL == (mqtt_data = mqtt_get_publish_data(message, &mqtt_data_length))

Would these changes work for you?

[gregjesl]

@david-cermak Disregard my last comment, it will fail for large messages.

This function should return true if the header is completely loaded in the buffer:

bool mqtt_header_complete(uint8_t* buffer, uint32_t blength)
{
    int i;
    int topiclen;

    for (i = 1; i < 5; ++i)
    {
        if(i >= blength)
            return false;
        if ((buffer[i] & 0x80) == 0)
        {
            ++i;
            break;
        }
    }
    // i is now the length of the fixed header

    if (i + 2 >= blength)
        return false;
    topiclen = buffer[i++] << 8;
    topiclen |= buffer[i++];

    if (i + topiclen > blength)
        return false;

    i += topiclen;

    if (mqtt_get_qos(buffer) > 0)
    {
        i += 2;
    }
    // i is now the length of the fixed + variable header
    return blength >= i;
}

[david-cermak]

@gregjesl Sounds good, that should work!

I was just writing about the issue with longer messages 😄

[gregjesl]

@david-cermak I've made the updates.

[david-cermak]

@gregjesl Thanks! This works fine and doesn't break existing tests. Have moved to internal repo for a review internally.

Meantime, could you please correct:

• if - else formatting per https://docs.espressif.com/projects/esp-idf/en/latest/contribute/style-guide.html#braces
• line 571 -- missing semicolon;

[gregjesl]

@david-cermak I've made the formatting changes to mqtt_client.c and fixed the semicolon issue. I did not adjust the formatting in mqtt_msg.c as it seems to match the formatting in that specific document.

Is there a way for me to test my changes before committing them besides altering the git submodule in the IDF? I didn't realize until today that the examples in the examples folder use the MQTT library in my IDF and not the ones in the local repository.

[david-cermak]

@gregjesl Thanks, yes I meant only the related changes to mqtt_client.c.

The easiest way to test it is to modify the submodule in IDF. You can then run the python tests in example folder. (there are other ways to include an external component, such as EXTRA_COMPONENT_DIR -- you can check .travis file in this repo to see how it is built, but I do not recommend)

[david-cermak]

Just two minor comments arose from the review

• use of that macro
• remove the "closing idf issue" from the commit message (I might have suggested otherwise in the beginning, but agree that the issue is to be closed upon submodule update in esp-idf not esp-mqtt, sorry for that)

@gregjesl I would prefer if you updated your changes, as it could be merged directly without me amending the commit.

[gregjesl]

@david-cermak That sounds good; I'll make the changes later today.

[gregjesl]

@david-cermak I've made the changes.

[david-cermak]

@gregjesl Many thanks for your contribution!