Erroneous response on getDescGeneric() may lead to an "index out of range" error

[samuelfmlourenco]

This issue can be confirmed when trying to access a CP2130 device, for instance. Although this class is specific to MCP2210 devices, it should not cause a crash when trying to perform functions on top of other devices. Here is the function as implemented:

// Private generic function that is used to get any descriptor
QString MCP2210::getDescGeneric(quint8 subcomid, int &errcnt, QString &errstr)
{
    QVector<quint8> command{
        GET_NVRAM_SETTINGS, subcomid  // Header
    };
    QVector<quint8> response = hidTransfer(command, errcnt, errstr);
    size_t maxLength = 2 * DESC_MAXLEN + 2;  // Maximum descriptor length in bytes
    size_t length = (response[4] > maxLength ? maxLength : response[4]) - 2;  // Descriptor internal length
    QString descriptor;
    for (size_t i = 0; i < length; i += 2) {
        descriptor += QChar(response[i + 7] << 8 | response[i + 6]);  // UTF-16LE conversion as per the USB 2.0 specification
    }
    return descriptor;
}

In this case, what happens, is that response[4] equals 0, and that causes an unsigned integer rollover, which in turns causes the iterator value inside the for loop to be incremented until the response QVector is read outside its boundaries. Preventing such rollover, although it would fix this issue in particular, it is not effective in fixing issues where response[4] returns any value greater than 56 (COMMAND_SIZE - 6). However, the fix can be done by just adding an extra condition to the for loop:

// Private generic function that is used to get any descriptor
QString MCP2210::getDescGeneric(quint8 subcomid, int &errcnt, QString &errstr)
{
    QVector<quint8> command{
        GET_NVRAM_SETTINGS, subcomid  // Header
    };
    QVector<quint8> response = hidTransfer(command, errcnt, errstr);
    size_t maxLength = 2 * DESC_MAXLEN + 2;  // Maximum descriptor length in bytes
    size_t length = (response[4] > maxLength ? maxLength : response[4]) - 2;  // Descriptor internal length
    QString descriptor;
    for (size_t i = 0; i < length && i < COMMAND_SIZE - 6; i += 2) {  // Extra condition added here!
        descriptor += QChar(response[i + 7] << 8 | response[i + 6]);  // UTF-16LE conversion as per the USB 2.0 specification
    }
    return descriptor;
}

The above solution is acceptable because the function should return gibberish, even when hidTransfer() returns an error (which it does when accessing a CP2130 device). Any error checking and filtering should be done outside the class, by checking both errcnt and errstr.

[samuelfmlourenco]

Solution as implemented in version 1.2.0:

// Private generic function that is used to get any descriptor
QString MCP2210::getDescGeneric(quint8 subcomid, int &errcnt, QString &errstr)
{
    QVector<quint8> command{
        GET_NVRAM_SETTINGS, subcomid  // Header
    };
    QVector<quint8> response = hidTransfer(command, errcnt, errstr);
    size_t maxSize = 2 * DESC_MAXLEN;  // Maximum size of the descriptor in bytes (the zero padding at the end takes two more bytes)
    size_t size = response[4] - 2;  // Descriptor actual size, excluding the padding
    size = size > maxSize ? maxSize : size;  // This also fixes an erroneous result due to a possible unsigned integer rollover (bug fixed in version 1.2.0)
    QString descriptor;
    for (size_t i = 0; i < size; i += 2) {
        descriptor += QChar(response[i + PREAMBLE_SIZE + 3] << 8 | response[i + PREAMBLE_SIZE + 2]);  // UTF-16LE conversion as per the USB 2.0 specification
    }
    return descriptor;
}