lib/vector/vlib: Fix possible null pointer dereference

[ymdatta]

In the function Vect_cat_list_to_array, as part of the execution, if list turns out to not contain any numbers, cats internal variable is not changed from NULL. Without checking if cats is NULL or not, qsort or first elemnt of it is accessed, which can lead to null pointer dereference.

To fix that issue, only access cats if it's not NULL.

This issue was found using cppcheck tool.

[nilason]

It seems to me to be a better solution to make an early exit before this for statement, with something like:

if (list->n_ranges <= 0)
    return -1;

If list->n_ranges is 0 or less, cats and n_cats are never set... and the rest doesn't make any sense.

@metzm Perhaps you may have some insight in this?

[ymdatta]

@nilason : Thanks for the review.

But, I am worried that '-1' indicates that something has gone wrong while converting using Vect_cat_list_to_array function, but here there is nothing wrong and it's just that the argument has no elements in it. What do you think about it?

[metzm]

I would suggest to make an early exit just before qsort, like

if (n_cats == 0) {
    return 0;
}

This is not a failure, there would be simply no entries in the list.

[Valyrian-Code]

Not a maintainer, just going through old unreviewed PRs. The bug is real, but I think the fix leaves a problem in the same path.

ucats is uninitialised at line 490 and this patch only assigns it inside if (cats). So when cats is NULL, which is the case being fixed, it still reaches G_realloc(ucats, ...) and *vals = ucats with the pointer never set. n_ucats = 0 makes the size 0 but the pointer argument is still garbage. Setting ucats = NULL next to cats = NULL would cover it.

Easiest trigger is a list straight from Vect_new_cat_list(), which sets n_ranges = 0, so the loop never runs.

Read from source, I have not built it.

[echoix]

Not a maintainer, just going through old unreviewed PRs. The bug is real, but I think the fix leaves a problem in the same path.

ucats is uninitialised at line 490 and this patch only assigns it inside if (cats). So when cats is NULL, which is the case being fixed, it still reaches G_realloc(ucats, ...) and *vals = ucats with the pointer never set. n_ucats = 0 makes the size 0 but the pointer argument is still garbage. Setting ucats = NULL next to cats = NULL would cover it.

Easiest trigger is a list straight from Vect_new_cat_list(), which sets n_ranges = 0, so the loop never runs.

Read from source, I have not built it.

Whats your opinion on #7835 compared to this one then?

[Valyrian-Code]

I think #7835 is better. It returns -1 for an empty list where this one returns 0 with an array the caller reads. copy.c:717 is the only in-tree caller and its else branch, for a NULL cat_list, copies everything, so an empty selection handed back as an array risks copying all categories instead of none.

#7835 also removes the two cppcheck suppressions, and this one would still need ucats = NULL to be correct.