Some functions "pretend" to be "safe" while being unsafe

[phobos2077]

• get/set_proto_data - uses byte-offsets and has no range checking. Suggestion:

  • Don't accept offset < 0.
  • Check proto type/subtype and don't accept offsets above the upper bounds.

• get/set_object_data - uses byte offsets and direct memory pointer for object, is not limited an any way. Available with UnsafeScripting disabled, but actually works exactly the same as read_xxx, write_xxx opcodes if you just pass 0 as object pointer...

  • Disable this function if UnsafeScripting is disabled, because it's impossible to know what type of object is being pointed to for proper offset range validation.
  • Mark as deprecated and remove from sfall.h. Who wants unsafe scripts should use normal unsafe scripting functions (just add object pointer and offsets together, duh).
  • Possibly add object-specific metarules with enum-based fields, just like get_ai_object_data or set_drugs_data.

This will break a few mods, but it is worth it, IMO:

• Unsafe functions like these won't work when porting to Fallout CE or similar, unless they never actually go out of object bounds and the other engine has perfect struct binary compatibility with original game.
• Just why have UnsafeScripting option in the first place if it doesn't actually disable unsafe scripting?

Of course if we want to somehow "protect" from malicious scripting, a lot of other things need to be considered (and not sure if it's really possible, given the game's engine is full of holes as it is). But I think we should at least discourage mod creators from using these not because of malicious intent, but simple errors in scripts that could lead to hard-to-debug crashes due to memory overwrites and reads at incorrect offsets.

[NovaRain]

It's easy to add if (offset >= 0) to get/set_proto_data and get/set_object_data, just a matter of a few extra lines of code.
As for get/set_object_data, there was a check for valid objects, but it got removed due to the introduction of combat_data as a quick'n'dirty way to access the data structure. I think we can re-add the basic FEEDFACE obj check and make both functions "free" only during combat, so they can still be used to access combat data from combat_data function or in COMBATDAMAGE hook. Yes, it still doesn't solve the underlying problems and possible crashes, but at least it makes things harder to screw up.

[phobos2077]

I don't think relying on implementation details of memory allocation is an acceptable solution.
Checking against negative offset wouldn't solve anything without an upper bound check, so can just as well leave it as is.

I see one solution that wouldn't break existing code:

• Hook into every place where objects are created and destroyed (actually destroyed with memfree)
• Maintain an unordered_map of active objects to the type of object (game object or combat data,etc.)
• When these opcodes are used, find an object's type from the map and use it for bounds checking.
• If object is not in the map - print error and stop the script.

If properly coded, this should make this one particular set of opcodes safe. However, some vanilla functions, notable obj_pid can still be used to read any memory address. So more will have to be done to solve the issue. (but I guess fixing vanilla functions should be a separate issue)