#539 GIUH: Adjust CDF search indexing to start from 1 to avoid underflowing input array

[PhilMiller]

When the found bounding entry is 0, the subsequent decrement means that we subscript the vector at -1, resulting in an out-of-bounds access. Detected with AddressSanitizer, confirmed by testing the code with .at() in place of [].

Fixes #539

Testing

1. ctest re-run after fix, all pass

Checklist

• PR has an informative and human-readable title
• Changes are limited to a single goal (no scope creep)
• Code can be automatically merged (no conflicts)
• Code follows project standards (link if applicable)
• Passes all existing automated tests
• Any change in functionality is tested
• New functions are documented (with a description, list of inputs, and expected output)
• Placeholder code is flagged / future todos are captured in comments
• Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
• Reviewers requested with the Reviewers tool ➡️

[PhilMiller]

Does this potentially change code results/output in some cases? I've only looked very narrowly at this code to make the change, so I'm relying on @robertbartel suggesting it to affirm that it's a reasonable thing to do.

[robertbartel]

Spending a little more time on this, I am not 100% certain this change is guaranteed to be correct for all possible cases, but I still believe it will suffice because this is effectively dead legacy code.

The comments in the code talk about finding index of "the first time greater," then backing up one index to get "the last smaller." This doesn't speak to what should happen for an index with an equal value, which is what we have in the test case that prompted this. Even the original GIUH calculations example doc I found isn't clear on what should happen in the case where a travel time (cdf_time) is equal to the current ordinate. So, it is possible simply skipping the first index isn't correct in certain rare cases.

On top of that, the example doc seems to start cdf_times with a value of 0 at index 0, but the test data starts with a non-zero value. I.e., I wonder if the problem follows from a flaw in the test data and whether we actually should not encounter this negative index scenario.

Regardless, I don't think we need to spend too much more time on this. The code in question is only still called by its own tests and our deprecated, non-BMI realization/formulation classes. I will leave it to @mattw-nws to decide (and to provide guidance on whether it's time to remove some of these old components), but for the issue at hand I think this is a suitable solution.

[mattw-nws]

@robertbartel Quite right, I'm afraid this is another instance of zombie code eating our brains. As discussed this morning, @PhilMiller if we can figure out how easy it will be to extricate this and remove it, that's probably the way to go here... this PR is probably worth keeping open until it's done, but until then probably don't worry about trying to resolve the OOB issue.

[PhilMiller]

If we think this is zombie code, could we just merge the ASan fix and not worry about its edge cases too much? I'd like to be able to put up a PR that applies the sanitizers in CI, and fails on error reports.

[mattw-nws]

Ah... ugh. Okay... fine by me, but this is how we keep finding ourselves back here. In the meantime, @robertbartel , do you see any way this makes anything worse, other than the zombie code problem? I think this was your point at the end of your last comment but I'll defer to you to approve this one from a technical perspective.

[robertbartel]

@mattw-nws, for this change to cause a problem would require:

1. a catchment configured to use one of the deprecated, hard-coded formulation types
2. a very particular configuration of GIUH data, with the first GIUH travel time value being greater than or equal to the regular time interval for the calculations

Item no. 1 is simply supposed to not occur. Moreover, I don't think there is any promise of accurate results at this point for the hard-coded formulations (or is there)?

Item no. 2 is outside my domain a bit. My impression was that it is unlikely, but it did occur in the test data (I don't know from where the test data was sourced).

[PhilMiller]

There's an alternative resolution of the problem that would let me move forward on ensuring the codebase passes all its tests with sanitizers enabled: disable the GIUH test in question.

No need investigation of whether/why this test is wrong, or whether anyone might be impacted.

[mattw-nws]

So lets conclude @robertbartel isn't prepared to approve based on the last comment. Disabling the particular test is essentially how we resolved the same kind of issue in #499 ... it's kicking the can down the road, but more explicitly choosing to (?). Lets go with that.

[PhilMiller]

Closed in favor of #553