0009139: fix invalid model ID in engine LOD functions cause a crash

[patrikjuvonen]

Mantis Bug Tracker issue
9139, fixes #446

Summary:

• An invalid model (at least 20000+) passed to engineSetModelLODDistance or engineGetModelLODDistance would cause a client crash. This PR will make sure the model is valid.

[patrikjuvonen]

Now that I think of it, are these supposed to support vehicle and ped models too?

[qaisjp]

does it work with vehicle and ped models?

[patrikjuvonen]

Seems like it does. I'll add those model checks in as well.

[qaisjp]

What kind of crash is caused, and where does it crash?

pModelInfo is currently (or should be) returning NULL for incorrect values so the current code (in master) should not be crashing.

If there is an underlying bug with CGameSA::GetModelInfo then that should be fixed instead.

[Pirulax]

It doesn't crash with invalid modelID, it just returns false.
At least for me.

[qaisjp]

This bug seems to be associated with https://github.com/multitheftauto/mtasa-blue/pull/155/files.

It returns true on this line: https://github.com/multitheftauto/mtasa-blue/pull/155/files#diff-3beea7b726cdd57f96a6d8226af4eb9fR538

Removing the offending line does not fix the problem. Returning false fixes the problem. Is return true a typo?

cc @Sergeanur @botder

[Sergeanur]

I don't quite remember, but I think CModelInfoSA::IsValid was used on animation request, so return true was required.

[patrikjuvonen]

I guess another option is to mark any 20000+ invalid (or make a separate IsValid method that doesn't care if it's 20000+...)

This is what I've been able to find out so far:

If the name starts with "txd", assume it's refering to a global txd. (https://github.com/multitheftauto/mtasa-blue/blob/master/Client/mods/deathmatch/logic/CModelNames.cpp#L10480)

Encode global txd as model id 20000+ (https://github.com/multitheftauto/mtasa-blue/blob/master/Client/mods/deathmatch/logic/CModelNames.cpp#L10498)

So some "global txd" is what 20000+ ids are, then causes a crash. No actual model has an id of 20000+ so I mean it must be related to some other internal SA fact how things work.

EDIT:
#784 (comment)
@forkerer commented:

As far as I'm aware, (and in the IDB i'm using), 20000 is original size of that array in gta sa, trying to change physical properties of anything above that crashes the client. I think the additional space reserved in CGameSA (up to 26000) was made either by mistake, or in case of mta team wanting to expand some limits.