Previously, the "this != NULL" checks in the isValid() methods were covering most of these cases, but they were removed as part of PR #274 because clang was generating warnings because calling a method on a null pointer is undefined behavior (so the condition *should* be tautological). This performs the checks outside of isValid(), which should avoid (at least this variety of) undefined behavior while fixing the crashes that dereferencing null pointers was causing.
ssm_create() subtracts the start coordinates from the position of the target, and then normalizes that. Since call-ssm-strike was using the position of the target as the start coordinates, this was obviously resulting in normalizing a null vec3d, generating the spurious warning. This just makes call-ssm-strike use a point immediately behind the target's position (using the target's orientation) as the starting coordinate, thereby avoiding the warning.
This bug was found by, of all people, woutersmits, while FREDding his happynewyear campaign. See http://www.hard-light.net/forums/index.php?topic=90069.0 for discussion. The crash was triggered by the use of ( validate-all-arguments ) as the entire sexp tree for one of his mission events.
This was a two-part fix. The first part was to ensure that every *-argument sexp (of which validate-all-arguments is but one) is actually used within a when-argument or every-time-argument tree. More specifically, every sexp operation that expects an argument handler should first check that the arg_handler node actually exists.
The second part was to partially revert one of Taylor's old commits:
Revision: 3644
Author: taylor
Date: Tuesday, October 24, 2006 9:17:24 AM
Message:
various bits of cleanup (warning messages, things that Valgrind complained about, etc.)
fix an out-of-bounds issue on Sexp_nodes[] which was leading to stack corruption
----
Modified : /branches/fs2_open_3_6_9/fs2_open/code/parse/sexp.cpp
This was intended to prevent a -1 array reference into Sexp_nodes[]. What Taylor did was to cause repeated calls of the parent_operator function to loop endlessly at the topmost parent. But there are certain edge cases (such as the one woutersmits discovered) where you need to know whether a parent is the topmost one. What Taylor should have done, and what this change does, is to keep the -1 as a valid return value but check for it in the places where it may cause an issue.
If the subsystem was <none>, sexp_beam_floating_fire() wasn't consuming the node, resulting in it also being parsed as the X coordinate (making it always zero, because atoi("<none>") == 0).
`multi_sexp_set_turret_primary_ammo()` and `multi_sexp_set_turret_secondary_ammo()` were using an uninitialized pointer instead of an uninitialized array, like every other instance of multi_get_string(). This is just begging to write to invalid memory.
This adds getter and setter SEXPs for turret ammo.
"turret-get-primary-ammo"
"turret-get-secondary-ammo"
"turret-set-primary-ammo"
"turret-set-secondary-ammo"
I also forgot to make turrets that don't have the "use multiple guns"
flag set switch banks if their first bank is empty, so that has been
fixed as well.
MageKing17