Skip to content

Do not delete g_pNeoLoading ourselves, probably already done by engine#730

Merged
nullsystem merged 1 commit into
NeotokyoRebuild:masterfrom
nullsystem:DoNotDeleteNeoLoadingOurselves
Oct 15, 2024
Merged

Do not delete g_pNeoLoading ourselves, probably already done by engine#730
nullsystem merged 1 commit into
NeotokyoRebuild:masterfrom
nullsystem:DoNotDeleteNeoLoadingOurselves

Conversation

@nullsystem

@nullsystem nullsystem commented Oct 14, 2024

Copy link
Copy Markdown
Collaborator

Description

Still getting crash here (user after free on dangling pointer?). Just don't handle it. It's also called on exit so it's unnecessary to even cleanup here anyway.

Toolchain

  • Linux GCC Distro Native Arch/GCC 14

@nullsystem nullsystem added the bug label Oct 14, 2024
@nullsystem nullsystem added this to the v9.1-prealpha milestone Oct 14, 2024
@nullsystem
nullsystem requested a review from a team October 14, 2024 17:33
@nullsystem
nullsystem requested a review from a team October 14, 2024 21:02
@Rainyan

Rainyan commented Oct 15, 2024

Copy link
Copy Markdown
Member

@nullsystem Do you have a crash dump for this, or repro steps?

@AdamTadeusz

Copy link
Copy Markdown
Contributor

It's probably not noticeable unless you specifically run the game from visual studio in debug mode maybe?
https://github.com/user-attachments/assets/670be906-ef59-4d55-b9ef-f7251a0113e6

@nullsystem

nullsystem commented Oct 15, 2024

Copy link
Copy Markdown
Collaborator Author

@Rainyan Repro step is just opening then closing, and it happens sometimes, sometimes it doesn't. If it happens it's on shutdown.

@AdamTadeusz Still a crash and rather anonying when on a debugger? Even for people on a release build and it happens, it'll end up filling up more dump files.

@AdamTadeusz

Copy link
Copy Markdown
Contributor

@Rainyan Repro step is just opening then closing, and it happens sometimes, sometimes it doesn't. If it happens it's on shutdown.

@AdamTadeusz Still a crash and rather anonying when on a debugger?

This is on master, not this branch. Haven't verified this fixes the problem yet but yes its a common crash on master on exit when debugging

@Rainyan

This comment was marked as outdated.

@Rainyan Rainyan left a comment

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.

Ok, I can reproduce it too. Looking at the backtrace, it seems to indeed be a segfault at line

g_pNeoLoading->MarkForDeletion();

with the ECX register ("this" ptr of the thiscall) pointing to unaddressable memory at the time of the function call, which then signals SIGSEGV.

Tracing the ~CNeoLoading base dtor, it's invoked via

from the engine library before the ~ClientModeShared dtor is called, so it seems to indeed be a use-after-free on the g_pNeoLoading pointer. And since the CNeoLoading destructor is already being invoked by the engine, we should be safe to remove these lines without a memory leak.

@nullsystem
nullsystem merged commit 57c1067 into NeotokyoRebuild:master Oct 15, 2024
@AdamTadeusz

Copy link
Copy Markdown
Contributor

firefox moment?

@nullsystem
nullsystem deleted the DoNotDeleteNeoLoadingOurselves branch May 7, 2025 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants