Skip to content
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.

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

Suspicious call to destructor #56

Closed
Try opened this issue Jan 29, 2023 · 5 comments
Closed

Suspicious call to destructor #56

Try opened this issue Jan 29, 2023 · 5 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@Try
Copy link
Contributor

Try commented Jan 29, 2023

https://github.com/lmichaelis/phoenix/blob/87235e137823460bebf2769a51fd6665ce3856ed/source/vm.cc#L266-L270

Found this while debugging "Chronicles of Myrthana" mod.
Relevant piece of code: https://github.com/lmichaelis/tcom-decompilation/blob/c993234e0fd8520401491674c8ababe461859d09/Source/Raw/526.d

func int SPELL_TELEKINESIS_FOCUS_REMOVER() {
    NPC = HLP_GETNPC(0x71b); // returns null. This is query for PC_HERE, but PC_HERE is not yet initialized
    NPC.FOCUS_VOB = 0; // invalid access, causing explicit destructor call 
    STAGE = 0;
    return FALSE;
}

Shortly after executing opcode::movi VM crashes with memory error.

@lmichaelis lmichaelis self-assigned this Jan 29, 2023
@lmichaelis lmichaelis added the bug Something isn't working label Jan 29, 2023
@lmichaelis lmichaelis added this to the v2.0.0 milestone Jan 29, 2023
@lmichaelis
Copy link
Member

Hi, thanks for the report. Looks like I missed that additional -- which is probably the incorrect part about this. I'll double-check tomorrow.

If you want to try to fix it right now, you can try swapping the -- to postfix instead of prefix. Not 100% sure that's the fix but it's worth a shot.

@Try
Copy link
Contributor Author

Try commented Jan 29, 2023

Hm, I've just commented the code - work fine. Again I'm not completely sure of the intent there, yet to me it look:

case opcode::movvf: {
	auto [ref, idx, context] = pop_reference(); // daedalus_stack_frame is deallocated here already

	if (!ref->is_member() || context != nullptr ||
		!(_m_flags & execution_flag::vm_allow_null_instance_access)) {
		// set and nothing else - fine
		ref->set_int(pop_int(), idx, context);
	} else if (ref->is_member()) {
		// should be nop?
	}

	break;
}

@lmichaelis
Copy link
Member

There are two frames at play here, since it's a movvf ("move function reference into variable"). First, there is the variable being moved into, and then there is the value being assigned (notice the pop_int()). This is why, if the check fails (so if the variable is a member, not instance is set and the VM does allow null-instance access), we need to destroy the next stack frame which should be at _m_stack[_m_stack_ptr].

lmichaelis added a commit that referenced this issue Jan 30, 2023
…tion

This fixes an issue with moving values into member variables where if the context is not set. Specifically, `movvf` and `movf` now handle this case correctly without corrupting the stack.
lmichaelis added a commit that referenced this issue Jan 30, 2023
The opcodes `addmovi`, `submovi`, `mulmovi`, `divmovi` now also check if the variable they're accessing is a member and the context is not set. This behavior is the same as with other `mov*` instructions
@lmichaelis lmichaelis added the awaiting verification The problem has been fixed, though external verification of this fix is required label Jan 30, 2023
@lmichaelis
Copy link
Member

lmichaelis commented Jan 30, 2023

Added fixes on main.

@Try
Copy link
Contributor Author

Try commented Jan 30, 2023

Verified - fix works. Thanks!

@Try Try closed this as completed Jan 30, 2023
@lmichaelis lmichaelis removed the awaiting verification The problem has been fixed, though external verification of this fix is required label Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants