QtUsb icon indicating copy to clipboard operation
QtUsb copied to clipboard

QUsbEndpoint::readData may throw std::bad_alloc exception.

Open jerry-yuan opened this issue 4 years ago • 0 comments

Describe the bug As following, QUsbEndpoint::readData(char*,quint64) will be invoked by QIODevice::read(quint64):

QByteArray QIODevice::read(qint64 maxSize)
{
    Q_D(QIODevice);
    QByteArray result;

#if defined QIODEVICE_DEBUG
    printf("%p QIODevice::read(%lld), d->pos = %lld, d->buffer.size() = %lld\n",
           this, maxSize, d->pos, d->buffer.size());
#endif

    // Try to prevent the data from being copied, if we have a chunk
    // with the same size in the read buffer.
    if (maxSize == d->buffer.nextDataBlockSize() && !d->transactionStarted
        && (d->openMode & (QIODevice::ReadOnly | QIODevice::Text)) == QIODevice::ReadOnly) {
        result = d->buffer.read();
        if (!d->isSequential())
            d->pos += maxSize;
        if (d->buffer.isEmpty())
             // this line will invoke with a null pointer as parameter
            readData(nullptr, 0);
        return result;
    }

    CHECK_MAXLEN(read, result);
    CHECK_MAXBYTEARRAYSIZE(read);

    result.resize(int(maxSize));
    qint64 readBytes = read(result.data(), result.size());
...

And in line 18 of QIODevice::read(quint64), the QUsbEndpoint::readData(char*,quint64) would be invoked with nullptr as the first parameter when the d->buffer is empty. While in QUsbEndpoint::readData(char*,quint64) there is an assert to make sure the data pointer is not null:

qint64 QUsbEndpoint::readData(char *data, qint64 maxSize)
{
    if (this->openMode() != ReadOnly)
        return -1;

    Q_D(QUsbEndpoint);
    DbgPrintFuncName();
   // this may throw a bad_alloc exception and kill the process.
    Q_CHECK_PTR(data);

    if (maxSize <= 0)
        return 0;

    QMutexLocker locker(&d->m_buf_mutex);
    qint64 read_size = d->m_buf.size();
...

PLatform:

  • OS: Windows
  • Version: 10
  • Qt: 5.12.9

To Reproduce Steps to reproduce the behavior:

  1. open an input endpoint
  2. let the usb device send n bytes
  3. read the n bytes by invoke QUsbEndpoint::read(quint64) twice:
endpoint->read(m);        // make sure m<n
endpoint->read(n-m);    // this line will crash the process
  1. The process will crash due to the assert.

tips: If you read the remain bytes in buffer by readAll instead, you will not receive this exception.

Expected behavior A clear and concise description of what you expected to happen. I'd like to read the entire data in several parts.

Additional context Add any other context about the problem here. I tried to search the points that invoke readData with nullptr as first parameter and find there are two points. These two points invoke the readData with 0 as second parameter. For this reason, I try to move Q_CHECK_PTR(data) to after the checking of maxSize:

    // Q_CHECK_PTR(data);  // this line is moved.
    if (maxSize <= 0)
        return 0;
    Q_CHECK_PTR(data);   

It seems work but I am not sure if this will lead to any other issues.

jerry-yuan avatar Jun 21 '21 12:06 jerry-yuan