X Tutup
Skip to content
/ scummvm Public

3DS: Fix improper memory freeing (Sprite.vertices)#6434

Merged
lephilousophe merged 1 commit intoscummvm:masterfrom
BallM4788:spritefix
Feb 16, 2025
Merged

3DS: Fix improper memory freeing (Sprite.vertices)#6434
lephilousophe merged 1 commit intoscummvm:masterfrom
BallM4788:spritefix

Conversation

@BallM4788
Copy link
Contributor

@BallM4788 BallM4788 commented Feb 13, 2025 •
edited
Loading

Up to this point, Graphics::Surface-derived Sprite objects have simply been decalred in the 3DS backend's osystem.h file. As such, the Sprite class's constructor was not being called, and the chunk of linear memory Sprite._vertices is meant to point to wasn't being allocated. This PR rectifies that.

The fact that the 3DS port has still been working to this point, despite writing into undefined space in main memory, is a happy accident. The undefined behavior only manifested during my efforts to make a framework with which 3DS hardware renderers can be created for 3D engines.

Sprite.vertices was being freed before it was meant to be freed.

@BallM4788 BallM4788 changed the title 3DS: Change backend's Sprite objects to pointers, create Sprites with 'new' @BallM4788 3DS: Fix faulty memory allocation (Sprite._vertices) Feb 14, 2025
@BallM4788 BallM4788 changed the title @BallM4788 3DS: Fix faulty memory allocation (Sprite._vertices) 3DS: Fix faulty memory allocation (Sprite._vertices) Feb 14, 2025
@lephilousophe
Copy link
Member

lephilousophe commented Feb 15, 2025

I don't think all of this makes sense.

The Sprite constructor must be called by the OSystem_3DS constructor and it is (I just checked the disassembly of N3DS::OSystem_3DS::OSystem_3DS).
This constructor is then called in main.

What makes you think the Sprite constructor is not running?

@BallM4788
Copy link
Contributor Author

BallM4788 commented Feb 15, 2025 •
edited
Loading

What makes you think the Sprite constructor is not running?

allocation_error_demo.txt
Take this diff file, git apply it to the master branch, compile, run and close, then check scummvm.log. What SHOULD happen is that ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~HERP DERP is printed every time the Sprite constructor is called, and sprite vertices size: 128 is printed every time Sprite.create is called (linearAlloc uses 0x80 alignment, so even though sizeof(vertex) * 4 is less than 128, 128 is what is allocated).

What is actually output, however, is this:

[2025-02-15 10:32:52] ScummVM 2.10.0git2362-g899ea70a209-dirty (Feb 15 2025 09:27:23)
[2025-02-15 10:32:52] TAINTED Tremor FLAC MP3 RGB zLib Theora AAC FreeType2 FriBiDi JPEG PNG VirtualKeyboard cloud (servers) TinyGL 
[2025-02-15 10:32:52] --- Log opened.
[2025-02-15 10:32:52] sprite vertices size: 0
[2025-02-15 10:32:52] 3ds initsize w:320 h:200
[2025-02-15 10:32:52] sprite vertices size: 0
[2025-02-15 10:32:53] Virtual keyboard pack 'vkeybd_small' loaded successfully
[2025-02-15 10:32:54] sprite vertices size: 0
[2025-02-15 10:32:54] sprite vertices size: 0
[2025-02-15 10:33:00] sprite vertices size: 0
[2025-02-15 10:33:00] --- Log closed successfully.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~HERP DERP is never printed, meaning that the Sprite constructor is never run.

Sprite.create prints sprite vertices size: 0 because linearGetSize returns 0 if no linear memory has been allocated at *mem, meaning once again that the Sprite constructor is never run.

With this PR, and with the following diff applied:
allocation_fix_demo.txt
the expected output is given:

[2025-02-15 11:10:07] ScummVM 2.10.0git2363-g72cd1a2dc63-dirty (Feb 15 2025 10:04:19)
[2025-02-15 11:10:07] TAINTED Tremor FLAC MP3 RGB zLib Theora AAC FreeType2 FriBiDi JPEG PNG VirtualKeyboard cloud (servers) TinyGL 
[2025-02-15 11:10:07] --- Log opened.
[2025-02-15 11:10:07] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~HERP DERP
[2025-02-15 11:10:07] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~HERP DERP
[2025-02-15 11:10:07] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~HERP DERP
[2025-02-15 11:10:07] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~HERP DERP
[2025-02-15 11:10:07] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~HERP DERP
[2025-02-15 11:10:08] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~HERP DERP
[2025-02-15 11:10:08] sprite vertices size: 128
[2025-02-15 11:10:08] 3ds initsize w:320 h:200
[2025-02-15 11:10:08] sprite vertices size: 128
[2025-02-15 11:10:08] Virtual keyboard pack 'vkeybd_small' loaded successfully
[2025-02-15 11:10:09] sprite vertices size: 128
[2025-02-15 11:10:09] sprite vertices size: 128
[2025-02-15 11:10:16] sprite vertices size: 128
[2025-02-15 11:10:18] sprite vertices size: 128
[2025-02-15 11:10:18] sprite vertices size: 128
[2025-02-15 11:10:18] sprite vertices size: 128
[2025-02-15 11:10:18] sprite vertices size: 128
[2025-02-15 11:10:18] sprite vertices size: 128
[2025-02-15 11:10:18] --- Log closed successfully.

@lephilousophe
Copy link
Member

lephilousophe commented Feb 15, 2025

You can't get the HERP DERP debug message because when Sprite is created (in the OSystem_3DS constructor), the debug facility is not yet setup (I got bitten by this on Android backend recently).
The logging facility is set up in initBackend.

About why linearGetSize returns the correct value, it's because the code is indeed wrong and you fixed it in your PR.
The vertices pointer is freed in Sprite::free alongside the pixels values while it's allocated in the Sprite constructor.
The Sprite::free function being called in Sprite::create, the pointer gets invalid and your linearGetSize call returns 0.

You just have to move the linearFree(vertices); call from Sprite::free to Sprite::~Sprite and you should be good to go.

lephilousophe
lephilousophe reviewed Feb 15, 2025
View reviewed changes
lephilousophe
lephilousophe reviewed Feb 15, 2025
View reviewed changes
backends/platform/3ds/sprite.cpp Outdated

void Sprite::free() {
linearFree(vertices);
memset(vertices, 0, sizeof(vertex) * 4);
Copy link
Member

@lephilousophe lephilousophe Feb 15, 2025

Choose a reason for hiding this comment

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

And this one too although resetting the value is not necessary.

@BallM4788
Copy link
Contributor Author

BallM4788 commented Feb 15, 2025 •
edited
Loading

You can't get the HERP DERP debug message because when Sprite is created (in the OSystem_3DS constructor), the debug facility is not yet setup (I got bitten by this on Android backend recently).

That's confusing to me. Why does HERP DERP print with my PR? There's only ever six times the Sprite constructor is called (once for each Sprite object during startup), so it's not like the debug facility is picking up some later calls. Scratch that, I understand now. The changed I made meant that the Sprite constructor was now called after the debug facility had been setup.

Anyway, I'll try your suggestion and see if that fixes the problem on my other work (my most recent commit caused it to go from this to this, and it was fixed by the changes in this PR).

@BallM4788
Copy link
Contributor Author

BallM4788 commented Feb 15, 2025

OK yeah, you're right. Just your suggestions fixed it. I'll change the PR.

@BallM4788 BallM4788 changed the title 3DS: Fix faulty memory allocation (Sprite._vertices) 3DS: Fix improper memory freeing (Sprite.vertices) Feb 15, 2025
lephilousophe
lephilousophe reviewed Feb 15, 2025
View reviewed changes
@lephilousophe
Copy link
Member

lephilousophe commented Feb 16, 2025

Thank you! Merging.

@lephilousophe lephilousophe merged commit 97129b9 into scummvm:master Feb 16, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lephilousophe lephilousophe lephilousophe left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@BallM4788 @lephilousophe
X Tutup