Skip to content

[0.4.x] libvisual: Fix palette use-after-free (when switching actors) #214

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.

Sign up for GitHub

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

Merged
hartwork merged 4 commits into from
Feb 13, 2023
Merged

[0.4.x] libvisual: Fix palette use-after-free (when switching actors) #214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0c312ec
libvisual: Fix heap-use-after-free on pallete when switching actors
hartwork Jan 24, 2023
33cbab7
libvisual: Fix code path that misses setting the video palette
hartwork Jan 28, 2023
dfec9fe
lv-tool: Apply a full palette every time the palette changes and only…
hartwork Jan 28, 2023
981b9a2
libvisual: Sync translation
hartwork Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions libvisual/libvisual/lv_bin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,7 @@ int visual_bin_switch_finalize (VisBin *bin)
visual_log (VISUAL_LOG_DEBUG, "Entering...");
if (bin->managed == TRUE)
visual_object_unref (VISUAL_OBJECT (bin->actor));
visual_video_set_palette (bin->actvideo, NULL);

@kaixiong kaixiong Jan 25, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hartwork, where does the use-after-free happen? This also seems like an odd place to reset the palette. I feel that there should be a better place to stick this line in.

@hartwork hartwork Jan 25, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaixiong what I observed was that the palette of one plug-in would be used by the next one, because nothing was resetting the video palette on actor cleanup (where the free is happening) and then the first time that update_all runs in lv-tool, the new plugin has not been asked for its palette yet. While writing that, maybe we need changes in two places: avoiding the stale palette pointer but also getting a fresh one in before the first render. I'm happy to put these wherever fits best. What do you think?

@kaixiong kaixiong Jan 27, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hartwork That sounds like a good approach. I'm wondering how much work that is though.

If that's a lot of effort, I'm okay with fixing it like this (it's a critical bug after all) but with a big FIXME comment added above it so it can be re-looked at later.

@hartwork hartwork Jan 28, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaixiong I went a bit further with this, let me know how you like the new version.


/* Copy over the depth to be sure, and for GL plugins */
/* bin->actvideo->depth = bin->actmorphvideo->depth;
Expand Down Expand Up @@ -851,6 +852,8 @@ int visual_bin_run (VisBin *bin)

visual_bin_switch_finalize (bin);

visual_video_set_palette (bin->actvideo, visual_actor_get_palette (bin->actor));

/* We can't start drawing yet, the client needs to catch up with
* the depth change */
return 0;
Expand Down
8 changes: 4 additions & 4 deletions libvisual/po/de.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ msgid ""
msgstr ""
"Project-Id-Version: libvisual 0.4.0-2.2\n"
"Report-Msgid-Bugs-To: https://github.com/Libvisual/libvisual/issues/\n"
"POT-Creation-Date: 2023-02-05 06:13+0100\n"
"POT-Creation-Date: 2023-02-06 03:03+0100\n"
"PO-Revision-Date: 2017-08-10 18:46+GMT\n"
"Last-Translator: Chris Leick <c.leick@vollbio.de>\n"
"Language-Team: German <debian-l10n-german@lists.debian.org>\n"
Expand Down Expand Up @@ -188,11 +188,11 @@ msgstr ""
msgid "depth of the main actor: %d"
msgstr "Tiefe des Hauptdarstellers: %d"

#: libvisual/lv_bin.c:739
#: libvisual/lv_bin.c:740
msgid "negotiate without event"
msgstr "handeln ohne Ereignis"

#: libvisual/lv_bin.c:741
#: libvisual/lv_bin.c:742
msgid "end negotiate without event"
msgstr "handeln ohne Ereignis beenden"

Expand Down Expand Up @@ -900,7 +900,7 @@ msgstr ""
msgid "Invalid depth passed to the scaler"
msgstr "Ungültige Tiefe an Scaler übergeben"

#: tools/lv-tool/lv-tool.cpp:882 tools/lv-tool/lv-tool.cpp:1063
#: tools/lv-tool/lv-tool.cpp:892 tools/lv-tool/lv-tool.cpp:1073
msgid "lv-tool"
msgstr ""

Expand Down
8 changes: 4 additions & 4 deletions libvisual/po/es_AR.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ msgid ""
msgstr ""
"Project-Id-Version: Libvisual Library 0.4.0\n"
"Report-Msgid-Bugs-To: https://github.com/Libvisual/libvisual/issues/\n"
"POT-Creation-Date: 2023-02-05 06:13+0100\n"
"POT-Creation-Date: 2023-02-06 03:03+0100\n"
"PO-Revision-Date: 2005-03-20 17:09-0300\n"
"Last-Translator: <dprotti@users.sourceforge.net>\n"
"Language-Team: Spanish\n"
Expand Down Expand Up @@ -186,11 +186,11 @@ msgstr "fin de cambio de actor por nombre ******************"
msgid "depth of the main actor: %d"
msgstr "profundidad para el actor principal: %d"

#: libvisual/lv_bin.c:739
#: libvisual/lv_bin.c:740
msgid "negotiate without event"
msgstr "negociando sin evento"

#: libvisual/lv_bin.c:741
#: libvisual/lv_bin.c:742
msgid "end negotiate without event"
msgstr "fin de negociación sin evento"

Expand Down Expand Up @@ -918,7 +918,7 @@ msgstr ""
msgid "Invalid depth passed to the scaler"
msgstr "Profundidad pasada al escalador inválida"

#: tools/lv-tool/lv-tool.cpp:882 tools/lv-tool/lv-tool.cpp:1063
#: tools/lv-tool/lv-tool.cpp:892 tools/lv-tool/lv-tool.cpp:1073
msgid "lv-tool"
msgstr ""

Expand Down
8 changes: 4 additions & 4 deletions libvisual/po/es_ES.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ msgid ""
msgstr ""
"Project-Id-Version: Libvisual Library 0.4.0\n"
"Report-Msgid-Bugs-To: https://github.com/Libvisual/libvisual/issues/\n"
"POT-Creation-Date: 2023-02-05 06:13+0100\n"
"POT-Creation-Date: 2023-02-06 03:03+0100\n"
"PO-Revision-Date: 2005-03-20 17:09-0300\n"
"Last-Translator: <dprotti@users.sourceforge.net>\n"
"Language-Team: Spanish\n"
Expand Down Expand Up @@ -186,11 +186,11 @@ msgstr "fin de cambio de actor por nombre ******************"
msgid "depth of the main actor: %d"
msgstr "profundidad para el actor principal: %d"

#: libvisual/lv_bin.c:739
#: libvisual/lv_bin.c:740
msgid "negotiate without event"
msgstr "negociando sin evento"

#: libvisual/lv_bin.c:741
#: libvisual/lv_bin.c:742
msgid "end negotiate without event"
msgstr "fin de negociación sin evento"

Expand Down Expand Up @@ -918,7 +918,7 @@ msgstr ""
msgid "Invalid depth passed to the scaler"
msgstr "Profundidad pasada al escalador inválida"

#: tools/lv-tool/lv-tool.cpp:882 tools/lv-tool/lv-tool.cpp:1063
#: tools/lv-tool/lv-tool.cpp:892 tools/lv-tool/lv-tool.cpp:1073
msgid "lv-tool"
msgstr ""

Expand Down
8 changes: 4 additions & 4 deletions libvisual/po/libvisual-0.4.pot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ msgid ""
msgstr ""
"Project-Id-Version: libvisual 0.4.1\n"
"Report-Msgid-Bugs-To: https://github.com/Libvisual/libvisual/issues/\n"
"POT-Creation-Date: 2023-02-05 06:13+0100\n"
"POT-Creation-Date: 2023-02-06 03:03+0100\n"
"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
"Language-Team: LANGUAGE <LL@li.org>\n"
Expand Down Expand Up @@ -186,11 +186,11 @@ msgstr ""
msgid "depth of the main actor: %d"
msgstr ""

#: libvisual/lv_bin.c:739
#: libvisual/lv_bin.c:740
msgid "negotiate without event"
msgstr ""

#: libvisual/lv_bin.c:741
#: libvisual/lv_bin.c:742
msgid "end negotiate without event"
msgstr ""

Expand Down Expand Up @@ -871,6 +871,6 @@ msgstr ""
msgid "Invalid depth passed to the scaler"
msgstr ""

#: tools/lv-tool/lv-tool.cpp:882 tools/lv-tool/lv-tool.cpp:1063
#: tools/lv-tool/lv-tool.cpp:892 tools/lv-tool/lv-tool.cpp:1073
msgid "lv-tool"
msgstr ""
26 changes: 18 additions & 8 deletions libvisual/tools/lv-tool/lv-tool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
#include "gettext.h"
#include <libvisual/libvisual.h>
#include <SDL.h>
#include <algorithm>
#include <array>
#include <stdexcept>
#include <iostream>
#include <unordered_set>
Expand Down Expand Up @@ -412,21 +414,29 @@ namespace {
void update_all() {
visual_log_return_if_fail( m_screen != nullptr );

static VisPalette * prev_pal = nullptr;
if (m_screen->format->BitsPerPixel == 8) {
visual_log_return_if_fail( m_screen_video != nullptr );
VisPalette * const pal = m_screen_video->pal;

if (pal != nullptr && pal->ncolors >= 256) {
SDL_Color colors[256];

for (int i = 0; i < 256; i++) {
colors[i].r = pal->colors[i].r;
colors[i].g = pal->colors[i].g;
colors[i].b = pal->colors[i].b;
if (pal != prev_pal) {
std::array<SDL_Color, 256> colors {};

if (pal) {
const std::size_t source_color_count = std::min(colors.size(),
static_cast<visual_size_t>(pal->ncolors));
for (std::size_t i = 0; i < source_color_count; i++) {
colors[i].r = pal->colors[i].r;
colors[i].g = pal->colors[i].g;
colors[i].b = pal->colors[i].b;
}
}

SDL_SetColors (m_screen, colors, 0, 256);
SDL_SetColors (m_screen, colors.data(), 0, colors.size());
prev_pal = pal;
}
} else {
prev_pal = nullptr;
}

if (m_requested_depth == VISUAL_VIDEO_DEPTH_GL) {
Expand Down