Skip to content

Conversation

@KyleSanderson
Copy link
Member

This one is likely a doozy (and not even compile tested). This is to fix the freeing during callback problems that plague us to throw an error immediately mid-callback. menusys, functions, timers, anything with a delayed callback can cause us problems. I think this is the best option after sitting on it for a couple days and reviewing this morning least invasive approach we can take.

#1041

@KyleSanderson KyleSanderson added Bug general bugs; can be anything Needs Testing untested by author WIP work in progress labels Jul 21, 2019
@KyleSanderson KyleSanderson requested a review from dvander July 21, 2019 16:42
@KyleSanderson KyleSanderson removed the WIP work in progress label Jul 21, 2019
@nosoop
Copy link
Contributor

nosoop commented Jul 21, 2019

👀 This looks much more robust.

I'll run it against my test case and report back on whether or not they crash when I'm not on snooze.

@dvander
Copy link
Member

dvander commented Jul 24, 2019

What problem is this solving?

@KyleSanderson
Copy link
Member Author

KyleSanderson commented Jul 24, 2019

What problem is this solving?

Immediate errors during a callback when it's actively in-use. Fyren mentioned we should continue to just paper over but an error here feels a little more appropriate (to me atleast, obviously debatable; the example of creating a timer to free memory or waiting to breakout of the callback isn't ideal).

@nosoop
Copy link
Contributor

nosoop commented Jul 27, 2019

Apologies for the delay.

Seems like it still crashes at the same spot in CForward::Execute; attached SM is built from 155248a in case that'd help if you also want to repro with the environment (only built with TF2).

Also have accelerator submissions: OXOR-TW5T-FB4A, 5IOG-N6TT-SK3F, 23U6-PPR7-UD7L.

sourcemod.tar.gz

Did some logging tests; from my understanding it seems like access_special isn't set on the handle so it's still pulling it from the handle type?

@KyleSanderson
Copy link
Member Author

@nosoop so, this is completely on me. I let this one fester and lay dormant (to the point of needing to be re-based). My apologies.

What's your current take on this one? What do you feel would be the best way forward here? Is this still an issue in SM?

@nosoop
Copy link
Contributor

nosoop commented Aug 17, 2020

I think this approach is fine once the access inheritance issue is ironed out, but I don't have knowledge of the handle system to have an opinion on other approaches. I'm also mostly indifferent about whether it throws an error or is handled transparently; I can see valid reasons for both.

I think it's still worth resolving on SourceMod's side — the most recent discussion related to this crash was on August 3 this year.

@KyleSanderson
Copy link
Member Author

@TheDS (or @dvander) how do you think we can fix this MM issue in SM? I still think taking a parent clone is the right path but I'm totally open to other solutions.

@KyleSanderson KyleSanderson marked this pull request as draft March 30, 2023 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug general bugs; can be anything Needs Testing untested by author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants