[vlc-devel] [PATCH] skins: fix signed overflow in bitmap allocation

[erwan.tulou at gmail.com]

Hello,

    just wondering ... is the skins2 module still expected to be part of 
the next major vlc release ?

   As of today, the skins2 module is broken (crashing). The last patch 
to try and fix it did not receive much attention. And it seems to me the 
Qt module is evolving without much care about how the skins2 module may 
be impacted.

   So, maybe time has come to ask the questions  :  is it worth 
maintaining it or not ?  what are the plans ? any new maintainers 
interested ?

Regards
Erwan

On 22/12/2020 18:10, remi at remlab.net wrote:
> From: Rémi Denis-Courmont <remi at remlab.net>
>
> Reported-by GitHub team members: @erik-krogh, @geoffw0, @MathiasVP, @owen-mc
> ---
>   modules/gui/skins2/src/file_bitmap.cpp    |  7 ++++++-
>   modules/gui/skins2/src/ft2_bitmap.cpp     | 10 ++++++++--
>   modules/gui/skins2/src/generic_bitmap.cpp | 10 ++++++++--
>   modules/gui/skins2/src/scaled_bitmap.cpp  |  8 ++++++--
>   4 files changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/modules/gui/skins2/src/file_bitmap.cpp b/modules/gui/skins2/src/file_bitmap.cpp
> index 658bdc122f..c9505be370 100644
> --- a/modules/gui/skins2/src/file_bitmap.cpp
> +++ b/modules/gui/skins2/src/file_bitmap.cpp
> @@ -38,6 +38,7 @@ FileBitmap::FileBitmap( intf_thread_t *pIntf, image_handler_t *pImageHandler,
>   {
>       video_format_t fmt_out;
>       picture_t *pPic;
> +    unsigned size;
>   
>       video_format_Init( &fmt_out, VLC_CODEC_RGBA );
>   
> @@ -61,7 +62,11 @@ FileBitmap::FileBitmap( intf_thread_t *pIntf, image_handler_t *pImageHandler,
>       m_height = fmt_out.i_height;
>       video_format_Clean( &fmt_out );
>   
> -    m_pData = new uint8_t[m_height * m_width * 4];
> +    if (mul_overflow((unsigned)m_width, (unsigned)m_height, &size)
> +     || mul_overflow(size, 4, &size))
> +        throw std::bad_alloc();
> +
> +    m_pData = new uint8_t[size];
>   
>       // Compute the alpha layer
>       uint8_t *pData = m_pData, *pSrc = pPic->p->p_pixels;
> diff --git a/modules/gui/skins2/src/ft2_bitmap.cpp b/modules/gui/skins2/src/ft2_bitmap.cpp
> index 08ce4a3421..7b1c468705 100644
> --- a/modules/gui/skins2/src/ft2_bitmap.cpp
> +++ b/modules/gui/skins2/src/ft2_bitmap.cpp
> @@ -27,9 +27,15 @@
>   FT2Bitmap::FT2Bitmap( intf_thread_t *pIntf, int width, int height ):
>       GenericBitmap( pIntf ), m_width( width ), m_height( height )
>   {
> +    unsigned size;
> +
> +    if (mul_overflow((unsigned)width, (unsigned)height, &size)
> +     || mul_overflow(size, 4, &size))
> +        throw std::bad_alloc();
> +
>       // Allocate memory for the buffer
> -    m_pData = new uint8_t[m_height * m_width * 4];
> -    memset( m_pData, 0, m_height * m_width * 4 );
> +    m_pData = new uint8_t[size];
> +    memset(m_pData, 0, size);
>   }
>   
>   
> diff --git a/modules/gui/skins2/src/generic_bitmap.cpp b/modules/gui/skins2/src/generic_bitmap.cpp
> index 60edecca40..a0c40f404a 100644
> --- a/modules/gui/skins2/src/generic_bitmap.cpp
> +++ b/modules/gui/skins2/src/generic_bitmap.cpp
> @@ -57,8 +57,14 @@ BitmapImpl::BitmapImpl( intf_thread_t *pIntf, int width, int height,
>       GenericBitmap( pIntf, nbFrames, fps, nbLoops ), m_width( width ),
>       m_height( height ), m_pData( NULL )
>   {
> -    m_pData = new uint8_t[width * height * 4];
> -    memset( m_pData, 0, width * height * 4 );
> +    unsigned size;
> +
> +    if (mul_overflow((unsigned)width, (unsigned)height, &size)
> +     || mul_overflow(size, 4, &size))
> +        throw std::bad_alloc();
> +
> +    m_pData = new uint8_t[size];
> +    memset(m_pData, 0, size);
>   }
>   
>   
> diff --git a/modules/gui/skins2/src/scaled_bitmap.cpp b/modules/gui/skins2/src/scaled_bitmap.cpp
> index a85877b381..0bcbd02c4c 100644
> --- a/modules/gui/skins2/src/scaled_bitmap.cpp
> +++ b/modules/gui/skins2/src/scaled_bitmap.cpp
> @@ -28,10 +28,14 @@ ScaledBitmap::ScaledBitmap( intf_thread_t *pIntf, const GenericBitmap &rBitmap,
>                               int width, int height ):
>       GenericBitmap( pIntf ), m_width( width ), m_height( height )
>   {
> -    // XXX We should check that width and height are positive...
> +    unsigned size;
> +
> +    if (mul_overflow((unsigned)width, (unsigned)height, &size)
> +     || mul_overflow(size, 4, &size))
> +        throw std::bad_alloc();
>   
>       // Allocate memory for the buffer
> -    m_pData = new uint8_t[m_height * m_width * 4];
> +    m_pData = new uint8_t[size];
>   
>       int srcWidth = rBitmap.getWidth();
>       int srcHeight = rBitmap.getHeight();