[PATCH] Crash fix in add_single_dive. No writing to dive_table.dives[-1]

[K. "pestophagous" Heller]

On Wed, Nov 25, 2015 at 6:53 AM, Lubomir I. Ivanov <neolit123 at gmail.com> wrote:
>>  subsurface-core/divelist.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/subsurface-core/divelist.c b/subsurface-core/divelist.c
>> index a14fabf..a2e94c0 100644
>> --- a/subsurface-core/divelist.c
>> +++ b/subsurface-core/divelist.c
>> @@ -790,6 +790,9 @@ void add_single_dive(int idx, struct dive *dive)
>>         dive_table.nr++;
>>         if (dive->selected)
>>                 amount_selected++;
>> +       if (idx < 0)
>> +               // convert an idx of -1 so we do insert-at-end:
>> +               idx = dive_table.nr - 1;
>>         for (i = idx; i < dive_table.nr; i++) {
>>                 struct dive *tmp = dive_table.dives[i];
>>                 dive_table.dives[i] = dive;
>
> currently add_single_dive() assumes a safe index.
> i think that add_single_dive() should not be touched, but instead the
> mobile app should be fixed (models bug?).
>

That sounds reasonable, but i would mention the following:

1. if add_single_dive is going to assume a safe index, then we should
add an assert. an assertion failure would have been preferable to
varied and mysterious crashes after the call.

2. the ability to call with -1 is also a feature. currently, if you
want to add a dive to the *front* of the list, you pass 0.  But if you
want to add it to the *end* of the list, you need to know the size of
the table (so you can pass the table size as your targeted insertion
point).  Passing 0 to mean "prepend" does not require knowledge of the
current table size.  So, using -1 as a shorthand for "append" provides
a similar feature, then. no need to know the table size.

/K