fix(streaming/five): unloading of weapon components

[packfile]

Resolves a crash caused by weapon components not being removed on data file unload (the UnloadDataFile method for CWeaponComponentDataFileMounter is a nullsub). This can be reproduced by creating a resource with e.g. 250 additional weapon components and restarting it 10 times, a pool error will eventually be hit. In our case, this was being hit in Rockstar Editor when playback was moving between clips

[packfile]

Using struct padding might be a bit risky in case of future struct changes

This was considered when doing the fix, given this structure's last field layout (according to rage-parser-dumps) hasn't changed since build 323 I figured it would be safe enough. What would be the preferred way of doing this in the future?

[Disquse]

This was considered when doing the fix, given this structure's last field layout

Yeah this is why it's acceptable, the risk is still kinda low. I guess the best thing we can do here (since the struct size was never changed before) is to add assert for weaponComponentInfoBlobClassSize value (_DEBUG only maybe?), at least it will bring our attention that this struct was changed.

[gottfriedleibniz]

Both CWeaponSwapData and CWeaponComponentReloadData are externally referenced structs and dangling pointers may exist after destructWeaponComponentBlob destroys component data. Technically, CWeaponComponentInfo would fall under this but CWeaponComponentGroupInfo is never used (IIRC).

This unsoundness should be documented at the very least. Although, it may be better to only allow component unloading if localMode or editorMode are set.

Also, the result to LoadDataFile should probably be checked.

[packfile]

Both CWeaponSwapData and CWeaponComponentReloadData are externally referenced structs and dangling pointers may exist after destructWeaponComponentBlob destroys component data.

Right, good catch. Before I go ahead with changes, are any issues foreseen with:

For CWeaponSwapData iterating all weapon animations and setting the WeaponSwapData to NULL. For CWeaponComponentReloadData iterating all weapon components and setting ReloadData to RELOAD_DEFAULT (Both above would only apply when its referencing a structure we're about to delete)

If the above is unwanted, then the localMode / editorMode check would suffice for our use case (R* editor crash fix).

[gottfriedleibniz]

With this solution, you may want to check if CWeaponComponentClip maintaining a reference to CWeaponComponentReloadLoopedData would cause any issues (edgiest of edgy edge-cases).

Unless sentry shows an issue needing to be fixed (e.g., players who swap servers sometimes hit this pool error), only enabling this for editorMode, localMode, etc. solves an immediate issue and does not require an additional cost (time) for little-to-no benefit. Never-to-be-addressed TODO statements are a universal solution.

This is already extreme edge-case territory and it being exploitable is unlikely (i.e., under very specific circumstances some garbage memory may be read). Ultimately, a higher authority will need to weigh-in.

(Sidenote: the CWeaponComponentInfoBlob destructor could be referenced from its parStructure. Although whatever is simpler/easier to implement is probably better).