[hartwork]

Can be reproduced by running lv-tool --exclude projectM,gforce --switch 10 with AddressSanitizer active.

[kaixiong]

@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]

@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]

@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]

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

[kaixiong]

@hartwork hmm.. I'm not sure I understand why this fixes the reported use-after-free.

On the change itself:

The entire 256-entry palette should be set, so if the source palette is smaller than that, the remaining entries should be assigned black. This is the original behaviour and #215 was where it was pointed out. Unfortunately, #213 was already applied and now 0.4 has the altered behaviour.

The other thing is that I'd prefer to use std::size_t instead of visual_size_t.

Finally, I think target_ncolors or target_color_count is better than 'target_colors`, a name that tends to suggest colour values, rather than a count of them.

[hartwork]

@hartwork hmm.. I'm not sure I understand why this fixes the reported use-after-free.

@kaixiong thanks for the review! Strictly speaking, there are three (related) things going on in here:

• Set the pointer to NULL that lived longer than it should have. That technically fixes the use-after-free already.
• The second things is that not only do we need to get rid of the old palette, we also need to get in the new applied. There was a code path where that didn't happen, previously. That fix is the second part.
• The third part is improving the upload of the palette to SDL, more on that below.

On the change itself:

The entire 256-entry palette should be set

Ack!

, so if the source palette is smaller than that, the remaining entries should be assigned black.

Ack!

This is the original behaviour

Not on 0.4.x, it copies full 256 entries or nothing at the moment:

libvisual/libvisual/tools/lv-tool/lv-tool.cpp

Lines 410 to 414 in 855f256

	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;
	}

and #215 was where it was pointed out.

Yes, but that was master and we didn't apply it because it was mistaken and you caught it during review 👍

Unfortunately, #213 was already applied and now 0.4 has the altered behaviour.

Pull request #213 fixed out-of-bounds access but did not change behavior beyond that. Now pull request #214 right here tries to get it right.

The other thing is that I'd prefer to use std::size_t instead of visual_size_t.

Can do, give me a minute.

Finally, I think target_ncolors or target_color_count is better than 'target_colors`, a name that tends to suggest colour values, rather than a count of them.

Ack, will adjust in a minute.

[hartwork]

The other thing is that I'd prefer to use std::size_t instead of visual_size_t.

Can do, give me a minute.

Finally, I think target_ncolors or target_color_count is better than 'target_colors`, a name that tends to suggest colour values, rather than a count of them.

Ack, will adjust in a minute.

@kaixiong done

[kaixiong]

@hartwork Thanks. I have another suggestion. Instead of having that complicated looking std::min() expression with a ternary operator, why not use an if block?

std::array<SDL_Color, 256> colors {};
if (pal) {
    const std::size_t pal_color_count = std::min(colors.size(), static_cast<visual_size_t>(pal->ncolors));
    
    for (std::size_t i = 0; i < pal_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.data(), 0, colors.size());

[hartwork]

@kaixiong that's a nice idea, done

[hartwork]

@kaixiong the Clang 16 CI issues here will go away after rebasing onto a merged #231.

[kaixiong]

@hartwork, sorry for the delay. Let me review the use-after-free fix soon.

[hartwork]

@hartwork, sorry for the delay. Let me review the use-after-free fix soon.

@kaixiong thank you! 👍

[hartwork]

@kaixiong do we need any more improvements here before a merge?

[kaixiong]

@kaixiong do we need any more improvements here before a merge?

@hartwork No it's all good. I was trying to find time to check the use-after-free fix and understand why it works.