Skip to content

Conversation

@nosoop
Copy link
Contributor

@nosoop nosoop commented Jul 16, 2019

PR is a fix for #1041.

Because the handle is only read once during sm_CallStartForward, and to avoid keeping an owned plugin's handle (where the plugin can delete it prior to finishing the call) and interfering with the behavior of sm_CallFinish (by changing the order of ResetCall() and pFunction->Execute()), I opted to clone it twice, which doesn't seem ideal.

I've marked this as a draft to get opinions. The unimplemented alternatives I can think of would be to either:

  • Hold a copy of the calling plugin's Handle_t value and ensure its validity at sm_CallFinish (which should be acceptable in theory; if the handle was freed prior to the call being finished I'd imagine it would also cause issues it causes a segfault anyways)
  • Free the handle outside of ResetCall() when SourceMod is finishing the call (I didn't want to modify ResetCall with a bool param if I didn't have to)

This prevents undefined behavior when a plugin deletes the handle to a currently running forward.
@KyleSanderson
Copy link
Member

I don't have time atm but the ideal fix is to mark MM (handlesys) as protected in weird circumstances like that and error on the second call.

@nosoop
Copy link
Contributor Author

nosoop commented Jul 17, 2019

I'm not sure what you mean by MM and marking it as protected; can't find anything in the handlesys-related core / public headers with that terminology.

Might have to defer delegate this PR to someone else if it touches on a lot of SourceMod internals (though I wouldn't mind looking into it further if you could point me in the right direction).

@nosoop
Copy link
Contributor Author

nosoop commented Jul 27, 2019

Closing as #1048 looks better suited for fixing the issue.

@nosoop nosoop closed this Jul 27, 2019
@nosoop nosoop deleted the clone-current-forward branch July 27, 2019 12:50
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.

2 participants