[Scummvm-tracker] [ScummVM :: Bugs] #16340: BACKENDS: Using Video::Subtitles with SDL surface backend will trigger undefined behavior

[ScummVM :: Bugs]

#16340: BACKENDS: Using Video::Subtitles with SDL surface backend will trigger
undefined behavior
------------------------+-----------------------
Reporter:  neuromancer  |       Owner:  (none)
    Type:  defect       |      Status:  new
Priority:  high         |   Component:  Graphics
 Version:               |  Resolution:
Keywords:  sdl          |        Game:
------------------------+-----------------------
Comment (by neuromancer):

 This patch avoid the ASAN crash:

 {{{
 diff --git a/video/subtitles.cpp b/video/subtitles.cpp
 index 116f8b4b43a..2085e28a217 100644
 --- a/video/subtitles.cpp
 +++ b/video/subtitles.cpp
 @@ -347,6 +347,22 @@ void Subtitles::loadSRTFile(const Common::Path
 &fname) {
  void Subtitles::setBBox(const Common::Rect &bbox) {
         _requestedBBox = bbox;

 +       // Sanity-check the bounding box dimensions to prevent integer
 overflows
 +       // in Surface::create, which takes shorts.
 +       if (_requestedBBox.width() < 0) {
 +               _requestedBBox.right = _requestedBBox.left;
 +       }
 +       if (_requestedBBox.height() < 0) {
 +               _requestedBBox.bottom = _requestedBBox.top;
 +       }
 +       // 32767 is SHRT_MAX.
 +       if (_requestedBBox.width() > 32767 - SHADOW * 2) {
 +               _requestedBBox.right = _requestedBBox.left + 32767 -
 SHADOW * 2;
 +       }
 +       if (_requestedBBox.height() > 32767 - SHADOW * 2) {
 +               _requestedBBox.bottom = _requestedBBox.top + 32767 -
 SHADOW * 2;
 +       }
 +
         Graphics::PixelFormat overlayFormat =
 g_system->getOverlayFormat();
         _overlayHasAlpha = overlayFormat.aBits() != 0;
         _surface->create(_requestedBBox.width() + SHADOW * 2,
 _requestedBBox.height() + SHADOW * 2, overlayFormat);
 @@ -400,26 +416,7 @@ bool Subtitles::drawSubtitle(uint32 timestamp, bool
 force, bool showSFX) {

                 // Recalculate the real bounding box to use
                 _realBBox = _requestedBBox;
 -
 -               if (_realBBox.bottom > height) {
 -                       // First try to move the bounding box
 -                       _realBBox.top -= _realBBox.bottom - height;
 -                       _realBBox.bottom = height;
 -               }
 -               if (_realBBox.top < 0) {
 -                       // Not enough space
 -                       _realBBox.top = 0;
 -               }
 -
 -               if (_realBBox.right > width) {
 -                       // First try to move the bounding box
 -                       _realBBox.left -= _realBBox.right - width;
 -                       _realBBox.right = width;
 -               }
 -               if (_realBBox.left < 0) {
 -                       // Not enough space
 -                       _realBBox.left = 0;
 -               }
 +               _realBBox.clip(Common::Rect(0, 0, width, height));

                 force = true;
         }
 @@ -522,7 +519,7 @@ void Subtitles::renderSubtitle() const {
                         currentX += partWidth;
                 }
                 height += _fonts["regular"]->getFontHeight();
 -               if (height + _vPad > _realBBox.bottom)
 +               if (height + _vPad > _realBBox.height())
                         break;
         }

 @@ -532,6 +529,10 @@ void Subtitles::renderSubtitle() const {
         _drawRect.top = 0;
         _drawRect.setWidth(totalWidth + SHADOW * 2);
         _drawRect.setHeight(height + SHADOW * 2);
 +
 +       // The drawRect is relative to the _surface, so we may need to
 clip it.
 +       Common::Rect surfaceRect(0, 0, _surface->w, _surface->h);
 +       _drawRect.clip(surfaceRect);
  }

  } // End of namespace Video

 }}}

 But it does not solve the issue with the frozen screen when the overlay is
 used.
-- 
Ticket URL: <https://bugs.scummvm.org/ticket/16340#comment:1>
ScummVM :: Bugs <https://bugs.scummvm.org>
ScummVM