[wesnoth-commits] [wesnoth/wesnoth] de3190: sdl: Prevent undefined behaviour in clip_rect_setter

[Iris Morelle]

  Branch: refs/heads/1.14
  Home:   https://github.com/wesnoth/wesnoth
  Commit: de3190e047a93124b746bea3a135469f3f59e70d
      https://github.com/wesnoth/wesnoth/commit/de3190e047a93124b746bea3a135469f3f59e70d
  Author: Iris Morelle <shadowm at wesnoth.org>
  Date:   2020-01-19 (Sun, 19 Jan 2020)

  Changed paths:
    M src/sdl/surface.hpp

  Log Message:
  -----------
  sdl: Prevent undefined behaviour in clip_rect_setter

Unlike what the ctor's documentation says, passing a null SDL_Rect* does
have unexpected consequences. First, one of the first two arguments to
SDL_IntersectRect will be a null pointer, which results in
SDL_IntersectRect returning SDL_FALSE without ever touching the output
SDL_Rect. In this context that means that SDL_SetClipRect will receive a
pointer to a structure full of uninitialized values and all kinds of
weirdness could ensue next depending on the phase of the moon.

Additionally, while the SDL functions called here will do nothing on a
null pointer to a surface, the check introduced here requires
dereferencing the surface's members, so we need to explicitly do nothing
if the surface is null.

Both cases don't seem to ever happen in practice, judging from a cursory
glance at how clip_rect_setter is currently used in the codebase, but
that doesn't mean they will never turn up in the future.


  Commit: fa48f62e23651c1290181c5eb9a3480a990a92d1
      https://github.com/wesnoth/wesnoth/commit/fa48f62e23651c1290181c5eb9a3480a990a92d1
  Author: Iris Morelle <shadowm at wesnoth.org>
  Date:   2020-01-19 (Sun, 19 Jan 2020)

  Changed paths:
    M src/display.cpp
    M src/help/help_text_area.cpp

  Log Message:
  -----------
  Do not use SDL render API for certain drawing operations

With the introduction of batched rendering in SDL 2.0.10, the behaviour
of the render API appears to have changed so that certain operations
forcibly queue a renderer clip rectangle set operation, using a default
rectangle if necessary. The command queue does not clean after itself
since it appears that the operating assumption is that you either use
the renderer API or you don't, and Wesnoth performs drawing operations
both with and without it in a few places.

With commit 4cbd6529e3d5f378d4dd0080dda8a47025b0d50d, our drawing
primitives were changed ("refactored") so that the render API is
forcibly used by them. When combined with contexts that use the
clip_rect_setter object, things get weird since the clipping rectangle
is reset behind the code's back.

As I see it, there's two solutions to this:

 1. Make clip_rect_setter use SDL_RenderSetClipRect(). The problem with
    this is that there are at least 3-4 places using clip_rect_setter on
    a target that isn't the screen framebuffer. Finding out whether the
    target surface *is* the screen seems like an inconvenient chore.

 2. Don't use the render API ever.

 3. Keep using the render API (a lot of other things do use it, such as
    the GUI2 canvas implementation) and change the few contexts where we
    use clip_rect_setter together with drawing primitives to call the
    SDL_Surface drawing primitives directly instead of using the render
    API.

This patch goes with option 3 since it seems the least intrusive. While
this fixes the two known cases of bug #4510 as of this writing (help
browser and minimap outline), I am not entirely sure if there are any
other users of clip_rect_setter hiding drawing primitive calls somewhere
underneath.

Also added relevant source comments in case someone decides to refactor
the involved code and break it again. It's especially necessary in the
minimap's case since we need to draw a grand total of 4 different
rectangles at once.

Fixes #4510.


  Commit: e2cdc9697ae690c8a15e69668384ab7250e8bfb4
      https://github.com/wesnoth/wesnoth/commit/e2cdc9697ae690c8a15e69668384ab7250e8bfb4
  Author: Iris Morelle <shadowm at wesnoth.org>
  Date:   2020-01-19 (Sun, 19 Jan 2020)

  Changed paths:
    M changelog.md

  Log Message:
  -----------
  Update changelog for issue #4510

[ci skip]


Compare: https://github.com/wesnoth/wesnoth/compare/4f680c97265b...e2cdc9697ae6