Out-of-bounds read when going to the race track as Pepper

[madebr]

I momentarily had a reproducer for an out-of-bounds read, but cannot reproduce it anymore. While looking into ways to trigger the bug, it somehow does not reproduce anymore. However, I believe the issue can come back at any time because the texture copy does not use the source texture size.

Reproducer:

• Build isle with -DISLE_ASAN=ON
• Start a new game
• Go to race track by dragging pepper's avator on top of the race track

Result:

ASAN failure at this memcpy in LegoTextureInfo::LoadBits.

Debugging shows the following parameters:

desc.dwWidth = 128
desc.dwWidth = 128

LegoTextureInfo::LoadBits is called here.

p_namedTexture->m_texture->m_image->m_surface.w = 128
p_namedTexture->m_texture->m_image->m_surface.h = 32

stack trace:

memcpy 0x00007ffff786eb5d
LegoTextureInfo::LoadBits legotextureinfo.cpp:201
FUN_1003f930 legoutils.cpp:782
Act1State::PlaceActors isle.cpp:1766
Isle::Enable isle.cpp:543
Isle::Notify isle.cpp:174
MxNotificationManager::Tickle mxnotificationmanager.cpp:108
MxTickleManager::Tickle mxticklemanager.cpp:56
IsleApp::Tick isleapp.cpp:955
SDL_AppIterate isleapp.cpp:326
SDL_IterateMainCallbacks SDL_main_callbacks.c:130
GenericIterateMainCallbacks SDL_sysmain_callbacks.c:51
SDL_EnterAppMainCallbacks_REAL SDL_sysmain_callbacks.c:62
SDL_EnterAppMainCallbacks SDL_dynapi_procs.h:207
SDL_main SDL_main_impl.h:59
SDL_RunApp_REAL SDL_runapp.c:40
SDL_RunApp_DEFAULT SDL_dynapi_procs.h:804
SDL_RunApp SDL_dynapi_procs.h:804
main SDL_main_impl.h:137
__libc_start_call_main 0x00007ffff644614a
__libc_start_main_impl 0x00007ffff644620b
_start 0x0000000000407ff5

[foxtacles]

Hmm, I wonder why this isn't always reproducible/ where the variance in behavior comes from. Seems like it should be deterministic

[madebr]

I think it has something to do with previous states. I can reproduce it with the maarten profile of this savegame: savegame.zip

[AJenbo]

There for sure are these issues in the info center related to state. I can still very reliably reproduce this https://github.com/isledecomp/isle-portable/issues/214 unless i do other things first.

[foxtacles]

What comes to mind, since you mentioned that width and height end up the same (128), is this logic:

https://github.com/isledecomp/isle-portable/blob/be73b40ae8bb16bd51d08fbfda2c823886b5f093/LEGO1/lego/sources/misc/legoimage.cpp#L105

p_square appears to always be set to zero when textures are read from the save file. However, when they are first read from the game assets, p_square depends on whether hardware rendering is enabled or not, and will then rescale the texture according to the logic there. I believe the bug may be if you use a save file created with a different renderer setting, and then the size ends up incorrect.

Just conjecture for now, haven't further evaluated if this is true but it would make sense given your numbers

[foxtacles]

@AJenbo opinion on whether the scaling to square textures is still relevant today? maybe dx5 at most?

From a save file compatibility perspective it would be nice if we could handle this dynamically and reverse the scaling if necessary

[foxtacles]

I believe the prerequisite to reproduce this, if the above logic issue is the cause, is to have a build the race car. The tail texture has a resolution of 128x32 and would be affected by this.

[AJenbo]

No it doesn't maks sens to make them square, I think it's based on a misunderstanding of older graphics cards requiring textures to be power of two, but that doesn't mean they have to be square.

Doing this in software is slow and leads to worse quality so it should really just be removed. I'm already, for the most part, handeling POT support in the back ends where it's relevant.

The fact that textures get embedded in save games in a bit crazy considering you can only pick from a fixed set, but there is no changing that at this point or it would break save games.

[foxtacles]

Ok. I'll try to reproduce this to confirm. If yes I'd be in favor to rescale the texture when loaded based on the expected destination size. This way we remain compatible with all save games out there.

[AJenbo]

Texture coordinates are in NDC (0-1.0) though so it shouldn't matter unless the game is trying to do something clever on top.

Or maybe it tires to copy it on to a texture of a different size when uploading?