[Development] QtCS 2017 QtCore sessions

[Thiago Macieira]

On quarta-feira, 1 de novembro de 2017 08:58:35 PDT Konstantin Tokarev wrote:
> > There are also no pathological cases since there is no overflow.
> 
> There is overflow, try e.g. QByteArray::fromBase64() with array of size
> larger than INT_MAX / 3

Everywhere where they may be overflow, you need to be careful and detect said 
overflow. Using signed or unsigned makes no difference: overflowing is bad.

> If size was unsigned such bugs wouldn't lead to crashes or potential
> security issues

Or they would. In QUtf8::convertFromUnicode:

    QByteArray result(len * 3, Qt::Uninitialized);

If size() were unsigned and larger than UINT_MAX / 3, then that multiplication 
would overflow. The result could potentially be a very small number.

    uchar *dst = reinterpret_cast<uchar *>(const_cast<char 
*>(result.constData()));
    const ushort *src = reinterpret_cast<const ushort *>(uc);
    const ushort *const end = src + len;
    while (src != end) {
        ...
    }

That loop never rechecks the size of the allocation. It simply assumes that 
the destination buffer is large enough to encode the worst case of the UTF-8

This means the use of unsigned would not prevent issues. The loop would still 
corrupt memory and could crash.

What's more, the unsigned overflow is not wrong. So you can't flag down the 
issue by using a sanitiser. If we keep it as signed, then that multiplication 
can be flagged and fixed.

qnumeric_p.h has add_overflow and mul_overflow for unsigned types. For a proper 
signed check, you really need help from the compiler. But we can add them on 
top of the unsigned ones for the ones that don't help (MSVC), by using the 
implementation-defined behaviour of how an unsigned number gets converted to 
signed.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center