Stop EntRefToEntIndex returning garbage if a bad parameter is passed#1323
Conversation
Seen multiple bad usage of this function that works only because whatever was passed in was returned as it wasnt an entity reference. This code should have worked and would be expected to have returned something invalid but instead the the input was returned which allowed the code to work when really it is bad code. See for one such case https://discordapp.com/channels/335290997317697536/335290997317697536/736518488314871868
There was a problem hiding this comment.
Thank you for the PR, however this passthrough behaviour is intentional - it takes what SourceMod calls a BCompatRef which can be either an entity reference or an entity index. This is important in the general case for handling values from functions that can be either networked or non-networked entities (but obviously not here, as non-networked can't be turned into an index).
Is there a user-facing bug you're looking to solve with this change? One thing I know has surprised people in the past is that the non-ent-ref path doesn't validate the index is valid - that is something I'd definitely be open to changing, but I think this change is a deviation from the general ent ref behaviour in the SM API and will cause widespread breakage.
Added the error text saying what shall be returned when a invalid parameter is passed.
|
My concern was more that it does it does not seem correct from a usage perspective as generally you would presume a entity reference and a index are seperate things, so when a none reference is passed it shouldnt return something that could be valid. I think validation should atleast be inplace here though and I can change this to be that, just from my understanding the above code shouldnt have worked but the entity index was already validated so it carried on its was through the code with no problems in this case. |
Not sure if it needs this much validation but this just mirrors how IsValidEntity works, so the entity index returned should be valid else INVALID_EHANDLE_INDEX is returned.
|
I've made it so that the return value for a passed entity index is now validated instead of just returning |
|
Asher is right, the previous code with GetEntData was ridiculously broken and has been largely discouraged for nearly a decade. There's still obvious performance issues here like GetEntPropArraySize in a loop. If you need help absolutely feel free to reach out on IRC, the forums, or Discord. |
|
This has nothing todo with GetEntData, this was about EntRefToEntIndex returning bad indexs if something invalid was passed @KyleSanderson think you got the wrong one :) |
asherkin
left a comment
asherkin
left a comment
There was a problem hiding this comment.
Thanks - that should hopefully save people some debugging pain. One minor doc point.
Feel free to expand the doc comment if you think it would help to further reduce confusion - a note that it can take an entity reference or index, and it'll validate that the returned index is valid and refers to the same entity probably wouldn't go amiss.
|
Sorry didnt specify that this should be done, less someone with more knowledge has any further suggestions for the documentation changes. |
|
@c0rp3n So, I don't want to leave this in-limbo as that's unfair to you. Asher's been traveling for a bit but we chatted about this one offline. My concern is in a Spawn hook IServerUnknown (what we call it atleast) is actually not assigned on some entities until after the entity is spawned. There's other situations (readas: a lot) where we have a valid reference / index but these member objects are not set by the game quite yet. There are detours and similar out there that hook even before Spawn (I remember in one-case I was even hooking edict allocations and doing all kinds of weird stuff to them in pawn). Basically, I'm worried that this is going to break existing plugins and multiple use-cases, I haven't reviewed what happens to logical entities with this but that's also a concern as they do not follow any traditional paths. @asherkin what do you think about adding an additional param, similar to what we did with GetClientAuthString, and defaulting it to "true" for validate. That way we can protect new consumers with a new set of includes, and old consumers will work as-is? |
|
@asherkin any update on this one? I still feel that this is very destructive as-is and will break many workflows. |
I'd like to still pursue this as the current behaviour causes no end of problems - I would happily accept (fixable) breakage here in order to get there, but that still needs careful evaluation. It sounds like you know a few concrete cases where this will break, can you show any of them breaking with the specific checks added here or is it just in cases of theoretical Source badness? |
|
Specifically with my setup I seem to recall spawnpoints being quite odd, on CS:S storing this was fine, and then in GO I had to nest a bunch of calls to get the same result (which would change with this). Ultimately the script can change, and this coming on on a major is fine just based on the current climate as it makes things "safer". |
|
Is there any consensus on wanting this? Happy to just close the issue, or make any changes. My original intent was to just avoid confusing behaviour. |
|
As far as I could tell, you had updated with the latest requested changes. I also looked through and think this fine. Thanks! |
… passed (alliedmodders#1323)" This reverts commit f9ad35b.
4 participants
Seen multiple bad usage of this function that works only because whatever was passed in was returned as it wasnt an entity reference.
This code should have worked and would be expected to have returned something invalid but instead the the input was returned which allowed the code to work when really it is bad code.
Really I would like this to return
-1orINVALID_ENT_REFERENCESee this code below
GetObjectreturns an entity index, this has been in TTT for years and really the code is malformed and should never have worked.You can see how long this has been present here.