Stop EntRefToEntIndex returning garbage if a bad parameter is passed

[c0rp3n]

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 -1 or INVALID_ENT_REFERENCE

See this code below GetObject returns an entity index, this has been in TTT for years and really the code is malformed and should never have worked.

iEntity = GetObject(client, false);

if (iEntity == -1)
{
    return;
}

iEntity = EntRefToEntIndex(iEntity);

if (iEntity == INVALID_ENT_REFERENCE)
{
    return;
}

You can see how long this has been present here.

[c0rp3n]

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.

[c0rp3n]

I've made it so that the return value for a passed entity index is now validated instead of just returning INVALID_EHANDLE_INDEX, this just mirrors the code involved with IsValidEntity.

[KyleSanderson]

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.

[c0rp3n]

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 :)

[c0rp3n]

Sorry didnt specify that this should be done, less someone with more knowledge has any further suggestions for the documentation changes.

[KyleSanderson]

@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?

[KyleSanderson]

@asherkin any update on this one? I still feel that this is very destructive as-is and will break many workflows.

[asherkin]

@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?

[KyleSanderson]

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".

[c0rp3n]

Is there any consensus on wanting this? Happy to just close the issue, or make any changes. Though I have been on a long hiatus from any SM dev work.

My original intent was to just avoid confusing behaviour. As it was something that tripped me up for a short while.

[psychonic]

As far as I could tell, you had updated with the latest requested changes. I also looked through and think this fine. Thanks!