Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GRAPHICS: fix off-by-one errors when drawing a rounded rectangle #3538

Merged
merged 1 commit into from Nov 21, 2021

Conversation

@benoit-pierre
Copy link
Contributor

@benoit-pierre benoit-pierre commented Nov 14, 2021

The code would create a rectangle of width+1 by weight+1, resulting in buffer overflows when drawing at the bottom limit of a surface.

The crash occurs when not using the builtin/classic theme, since 6850297 (which adds 2 buttons at the bottom of the GUI):

==280335==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x1488d826e86c at pc 0x562c57f47f34 bp 0x7ffc65483590 sp 0x7ffc65483580
WRITE of size 2 at 0x1488d826e86c thread T0
    #0 0x562c57f47f33 in void Graphics::colorFill<unsigned short>(unsigned short*, unsigned short*, unsigned short) graphics/VectorRendererSpec.cpp:452
    #1 0x562c57f0da70 in Graphics::VectorRendererAA<unsigned short>::drawInteriorRoundedSquareAlg(int, int, int, int, int, unsigned short, Graphics::VectorRenderer::FillMode) graphics/VectorRendererSpec.cpp:4210
    #2 0x562c57f0895f in Graphics::VectorRendererAA<unsigned short>::drawRoundedSquareAlg(int, int, int, int, int, unsigned short, Graphics::VectorRenderer::FillMode) graphics/VectorRendererSpec.cpp:4248
    #3 0x562c57efed01 in Graphics::VectorRendererSpec<unsigned short>::drawRoundedSquare(int, int, int, int, int) graphics/VectorRendererSpec.cpp:1289
    #4 0x562c57ba8c94 in Graphics::VectorRenderer::drawCallback_ROUNDSQ(Common::Rect const&, Graphics::DrawStep const&) graphics/VectorRenderer.h:433
    #5 0x562c57ef66c0 in Graphics::VectorRenderer::drawStep(Common::Rect const&, Common::Rect const&, Graphics::DrawStep const&, unsigned int) graphics/VectorRenderer.cpp:59
    #6 0x562c57b62c95 in GUI::ThemeEngine::drawDD(GUI::DrawData, Common::Rect const&, unsigned int, bool) gui/ThemeEngine.cpp:953
    #7 0x562c57b63b23 in GUI::ThemeEngine::drawButton(Common::Rect const&, Common::U32String const&, GUI::ThemeEngine::State, unsigned short) gui/ThemeEngine.cpp:1012
    #8 0x562c57bbb1dc in GUI::PicButtonWidget::drawWidget() gui/widget.cpp:654
    #9 0x562c57bb36e6 in GUI::Widget::draw() gui/widget.cpp:139
    #10 0x562c57a98400 in GUI::Dialog::drawWidgets() gui/dialog.cpp:189
    #11 0x562c57a982a1 in GUI::Dialog::drawDialog(GUI::DrawLayer) gui/dialog.cpp:177
    #12 0x562c57aa0489 in GUI::GuiManager::redraw() gui/gui-manager.cpp:388
    #13 0x562c57aa0e19 in GUI::GuiManager::runLoop() gui/gui-manager.cpp:463
    #14 0x562c57aaec6e in GUI::LauncherDialog::run() gui/launcher.cpp:301
    #15 0x562c57ab7bad in GUI::LauncherChooser::runModal() gui/launcher.cpp:938
    #16 0x562c577f1b46 in launcherDialog base/main.cpp:107
    #17 0x562c577f86a9 in scummvm_main base/main.cpp:573
    #18 0x562c577edf28 in main backends/platform/sdl/posix/posix-main.cpp:45
    #19 0x148910f5cb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
    #20 0x562c5774724d in _start ([…]/scummvm+0x5df24d)

0x1488d826e86c is located 108 bytes to the right of 512000-byte region [0x1488d81f1800,0x1488d826e800)
allocated by thread T0 here:
    #0 0x148912c8f459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
    #1 0x562c57ebcfd1 in Graphics::Surface::create(short, short, Graphics::PixelFormat const&) graphics/surface.cpp:76
    #2 0x562c57e9d1be in Graphics::ManagedSurface::create(short, short, Graphics::PixelFormat const&) graphics/managed_surface.cpp:153
    #3 0x562c57b5cbc0 in GUI::ThemeEngine::setGraphicsMode(GUI::ThemeEngine::GraphicsMode) gui/ThemeEngine.cpp:457
    #4 0x562c57b5b209 in GUI::ThemeEngine::init() gui/ThemeEngine.cpp:328
    #5 0x562c57a9f6fc in GUI::GuiManager::loadNewTheme(Common::String, GUI::ThemeEngine::GraphicsMode, bool) gui/gui-manager.cpp:291
    #6 0x562c57a9b957 in GUI::GuiManager::GuiManager() gui/gui-manager.cpp:98
    #7 0x562c577fa7e3 in Common::Singleton<GUI::GuiManager>::makeInstance() common/singleton.h:61
    #8 0x562c577fa424 in Common::Singleton<GUI::GuiManager>::instance() common/singleton.h:84
    #9 0x562c577f5cc7 in setupGraphics base/main.cpp:374
    #10 0x562c577f85d1 in scummvm_main base/main.cpp:532
    #11 0x562c577edf28 in main backends/platform/sdl/posix/posix-main.cpp:45
    #12 0x148910f5cb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)

SUMMARY: AddressSanitizer: heap-buffer-overflow graphics/VectorRendererSpec.cpp:452 in void Graphics::colorFill<unsigned short>(unsigned short*, unsigned short*, unsigned short)
Shadow bytes around the buggy address:
  0x02919b045cb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x02919b045cc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x02919b045cd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x02919b045ce0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x02919b045cf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x02919b045d00: fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]fa fa
  0x02919b045d10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x02919b045d20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x02919b045d30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x02919b045d40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x02919b045d50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==280335==ABORTING
The code would create a rectangle of width+1 by weight+1, resulting
in buffer overflows when drawing at the bottom limit of a surface.
@aquadran
Copy link
Member

@aquadran aquadran commented Nov 20, 2021

I tried to run with valgrind on current code base, I didn't find issue. is this still valid?

Loading

@benoit-pierre
Copy link
Contributor Author

@benoit-pierre benoit-pierre commented Nov 20, 2021

Yes, I can still reproduce with current master (e967c16), and compiling with --enable-asan.

Loading

@aquadran
Copy link
Member

@aquadran aquadran commented Nov 20, 2021

I can not reproduce with asan

Loading

@benoit-pierre
Copy link
Contributor Author

@benoit-pierre benoit-pierre commented Nov 20, 2021

Are you sure you're using the right theme (modern)? And the last version of that theme? I'm using the following commands:

▸ CXX='ccache g++' CC='ccache gcc' ./configure --disable-release --disable-all-engines --enable-engines=scumm --prefix=/usr --enable-debug --enable-asan
▸ make -j5
▸ ./scummvm --themepath=./gui/themes --gui-theme=scummmodern

Loading

@sev-
Copy link
Member

@sev- sev- commented Nov 21, 2021

@benoit-pierre what is the window size for you? also, which renderer are you using? OpenGL? In general, it would be useful if you provide [scummvm] section of your scummvm.ini as we cannot reproduce it here

Loading

@benoit-pierre
Copy link
Contributor Author

@benoit-pierre benoit-pierre commented Nov 21, 2021

I've not changed the defaults, so it looks like SDL Surface is used: starting with --gfx-mode=surfacesdl triggers the crash. The window size is 640x400.

There's no crash with --gfx-mode=opengl, but with that mode the bottom left icons are not drawn at the bottom limit (the window is bigger: 960x720).

Loading

@sev-
Copy link
Member

@sev- sev- commented Nov 21, 2021

Yep, now I got this reproduced. 640x400, GUI scale: medium

Loading

@sev-
Copy link
Member

@sev- sev- commented Nov 21, 2021

Phew, it took a substantial effort to look into it, because our GUI debugging code got bitrot in the meantime.

In short, this is a completely incorrect fix: there is no problem with rounded squares, but a problem with the theme itself. It just tries to draw beyond the screen height by 6 pixels in this case.

The problem lies in the layout, as it now has too many widgets vertically. I will fix the layout and will think about how to implement clipping properly so it does not crash like this in the future.

Loading

@sev- sev- closed this Nov 21, 2021
@benoit-pierre
Copy link
Contributor Author

@benoit-pierre benoit-pierre commented Nov 21, 2021

That's really not what I'm seeing: the call is made to draw right at the limit (y1+h equal _baseSurface.h), but because the code draw the square one pixel bigger than asked (width and height), there's a buffer overflow.

Loading

@sev-
Copy link
Member

@sev- sev- commented Nov 21, 2021

It is pure coincidence.

I fixed the problem by reducing the vertical space of spacers. Please check the latest master.

Loading

@sev- sev- reopened this Nov 21, 2021
@sev-
Copy link
Member

@sev- sev- commented Nov 21, 2021

Actually, after giving it a second look, even after I fixed the rootcause, your code will prevent the future crashes, so I am merging it as well.

Loading

@sev- sev- merged commit 2d8e057 into scummvm:master Nov 21, 2021
14 of 15 checks passed
Loading
@sev-
Copy link
Member

@sev- sev- commented Nov 21, 2021

...and I had to revert it. :/ It was rendering tons of artefacts across the whole theme.

Let me explain what is going on here.

Our layout code is quite stupid, and in case of overflow, like what happened in this case, it tries to draw off-screen. We turn off clipping for dialogs and that led to this crash. The total height is 407 pixels, thus, the bottom pictures just do not fit.

We need to implement clipping properly in order to avoid this bug.

And in the meantime, e.g. since the vector renderer was implemented, all the code in fact depends on those rectangles be one pixel bigger and applying this patch leads to doubled lines in most of the widgets.

Loading

@benoit-pierre
Copy link
Contributor Author

@benoit-pierre benoit-pierre commented Nov 21, 2021

Well yeah, if the theme depends on the implementation not respecting the boundaries passed as parameter, that's a bug too...

Loading

@benoit-pierre
Copy link
Contributor Author

@benoit-pierre benoit-pierre commented Nov 29, 2021

You could just reduce the width/height in each implementations by one:

diff --git a/graphics/VectorRendererSpec.cpp b/graphics/VectorRendererSpec.cpp
--- a/graphics/VectorRendererSpec.cpp
+++ b/graphics/VectorRendererSpec.cpp
@@ -1275,9 +1275,9 @@ drawRoundedSquare(int x, int y, int r, int w, int h) {
 	bool useOriginal = _clippingArea.contains(Common::Rect(x, y, x + w, y + h));
 
 	if (Base::_fillMode != kFillDisabled && Base::_shadowOffset
-		&& x + w + Base::_shadowOffset + 1 < Base::_activeSurface->w
-		&& y + h + Base::_shadowOffset + 1 < Base::_activeSurface->h
-		&& h > (Base::_shadowOffset + 1) * 2) {
+		&& x + w + Base::_shadowOffset < Base::_activeSurface->w
+		&& y + h + Base::_shadowOffset < Base::_activeSurface->h
+		&& h > Base::_shadowOffset * 2) {
 		if (useOriginal) {
 			drawRoundedSquareShadow(x, y, r, w, h, Base::_shadowOffset);
 		} else {
@@ -3132,6 +3132,9 @@ drawBorderRoundedSquareAlg(int x1, int y1, int r, int w, int h, PixelType color,
 	int pitch = _activeSurface->pitch / _activeSurface->format.bytesPerPixel;
 	int sw = 0;
 
+	w -= 1;
+	h -= 1;
+
 	PixelType *ptr_tl = (PixelType *)Base::_activeSurface->getBasePtr(x1 + r, y1 + r);
 	PixelType *ptr_tr = (PixelType *)Base::_activeSurface->getBasePtr(x1 + w - r, y1 + r);
 	PixelType *ptr_bl = (PixelType *)Base::_activeSurface->getBasePtr(x1 + r, y1 + h - r);
@@ -3196,6 +3199,9 @@ drawBorderRoundedSquareAlgClip(int x1, int y1, int r, int w, int h, PixelType co
 	int pitch = _activeSurface->pitch / _activeSurface->format.bytesPerPixel;
 	int sw = 0, sp = 0, hp = h * pitch;
 
+	w -= 1;
+	h -= 1;
+
 	PixelType *ptr_tl = (PixelType *)Base::_activeSurface->getBasePtr(x1 + r, y1 + r);
 	PixelType *ptr_tr = (PixelType *)Base::_activeSurface->getBasePtr(x1 + w - r, y1 + r);
 	PixelType *ptr_bl = (PixelType *)Base::_activeSurface->getBasePtr(x1 + r, y1 + h - r);
@@ -3259,6 +3265,8 @@ drawBorderRoundedSquareAlgClip(int x1, int y1, int r, int w, int h, PixelType co
 template<typename PixelType>
 void VectorRendererSpec<PixelType>::
 drawInteriorRoundedSquareAlg(int x1, int y1, int r, int w, int h, PixelType color, VectorRenderer::FillMode fill_m) {
+	w -= 1;
+	h -= 1;
 	// Do not draw empty space rounded squares.
 	if (w <= 0 || h <= 0) {
 		return;
@@ -3331,6 +3339,8 @@ drawInteriorRoundedSquareAlg(int x1, int y1, int r, int w, int h, PixelType colo
 template<typename PixelType>
 void VectorRendererSpec<PixelType>::
 drawInteriorRoundedSquareAlgClip(int x1, int y1, int r, int w, int h, PixelType color, VectorRenderer::FillMode fill_m) {
+	w -= 1;
+	h -= 1;
 	// Do not draw empty space rounded squares.
 	if (w <= 0 || h <= 0) {
 		return;
@@ -3662,6 +3672,9 @@ void VectorRendererSpec<PixelType>::
 drawRoundedSquareShadow(int x1, int y1, int r, int w, int h, int offset) {
 	int pitch = _activeSurface->pitch / _activeSurface->format.bytesPerPixel;
 
+	w -= 1;
+	h -= 1;
+
 	// "Harder" shadows when having lower BPP, since we will have artifacts (greenish tint on the modern theme)
 	uint8 expFactor = 3;
 	uint16 alpha = (_activeSurface->format.bytesPerPixel > 2) ? 4 : 8;
@@ -3753,6 +3766,9 @@ void VectorRendererSpec<PixelType>::
 drawRoundedSquareShadowClip(int x1, int y1, int r, int w, int h, int offset) {
 	int pitch = _activeSurface->pitch / _activeSurface->format.bytesPerPixel;
 
+	w -= 1;
+	h -= 1;
+
 	// "Harder" shadows when having lower BPP, since we will have artifacts (greenish tint on the modern theme)
 	uint8 expFactor = 3;
 	uint16 alpha = (_activeSurface->format.bytesPerPixel > 2) ? 4 : 8;
@@ -4042,6 +4058,9 @@ drawBorderRoundedSquareAlg(int x1, int y1, int r, int w, int h, PixelType color,
 	frac_t T = 0, oldT;
 	uint8 a1, a2;
 
+	w -= 1;
+	h -= 1;
+
 	PixelType *ptr_tl = (PixelType *)Base::_activeSurface->getBasePtr(x1 + r, y1 + r);
 	PixelType *ptr_tr = (PixelType *)Base::_activeSurface->getBasePtr(x1 + w - r, y1 + r);
 	PixelType *ptr_bl = (PixelType *)Base::_activeSurface->getBasePtr(x1 + r, y1 + h - r);
@@ -4118,8 +4137,8 @@ drawBorderRoundedSquareAlg(int x1, int y1, int r, int w, int h, PixelType color,
 template<typename PixelType>
 void VectorRendererAA<PixelType>::
 drawInteriorRoundedSquareAlg(int x1, int y1, int r, int w, int h, PixelType color, VectorRenderer::FillMode fill_m) {
-	w -= 2*Base::_strokeWidth;
-	h -= 2*Base::_strokeWidth;
+	w -= 1 + 2*Base::_strokeWidth;
+	h -= 1 + 2*Base::_strokeWidth;
 
 	// Do not draw empty space rounded squares.
 	if (w <= 0 || h <= 0) {

Not particularly pretty, but this ensure the stay the same.

Before:
04_original_rounded_aa
After:
05_fixed_rounded_aa

And without anti-aliasing, before:
02_original_rounded

After:
03_fixed_rounded

Some buttons might then need tweaking: e.g the "clear search" button now looks off.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants