lib/vector: Fix null pointer dereference for an empty cat_list

[Pranav-error]

Vect_cat_list_to_array() allocates its array inside the loop over the ranges, so a
list with no ranges leaves cats NULL, and last_cat = ucats[0] = cats[0] then
dereferences it.

An empty list is reachable through the public API: Vect_new_cat_list() creates a list
with no ranges, and Vect_copy_table_by_cat_list() checks only that the list is not
NULL, not that it has any.

This returns an empty array instead. The documented contract says -1 is failure, and an
empty selection isn't a failure, so it returns 0 with nvals set to 0. Happy to make it
-1 instead if you would rather callers treat an empty list as an error.

The guard is exact: n_cats only ever grows by n >= 1, so n_cats == 0 happens
exactly when the loop never ran, which is also the only case where cats is still NULL.

cppcheck has this under # True positives in .cppcheck-suppressions (as lines 513 and
517, which have since drifted to 515); both entries are removed here.

Tests: lib/vector/Vlib/tests/lib_vector_cats_test.py covers the empty list and the
sorting and deduplication that must not change. Run the way CI runs pytest, the empty-list
test segfaults without the guard and passes with it.

The tests need a session, so they come with a conftest.py whose fixture calls
gs.setup.init(project) without env=os.environ.copy(). The copy is right for the
subprocess-based tests but not here, since the library reads the environment in this
process.

I used an AI assistant while investigating and writing this. I understand the change and
can explain it.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[echoix]

I restarted the timeout in macOS run

[Pranav-error]

Thanks. For what it is worth, the two new tests take about 0.4s for the file locally, so they should not be adding anything meaningful to the macOS run time — but tell me if it times out again and I will dig into it.

[Pranav-error]

The Windows failure was mine, sorry — not flakiness. The test called the library through ctypes in the pytest process, so the null dereference ended the whole run rather than one test, and the Windows job stopped at 8%.

Pushed a fix: the library calls now run in a subprocess, so a crash shows up as a return code the test asserts on instead of taking the run down. Without the guard the test now fails with return code -11; with it, it passes. The conftest is back to the usual gs.setup.init(project, env=os.environ.copy()) pattern, since the subprocess gets the session environment explicitly and nothing has to be applied to the test process itself.

The macOS run looks separate: attempt 2 was 1215 passed with 29 errors, all Timeout (>300.0s) in temporal/t.info/tests/, nothing from this change.

[echoix]

An AI review (Opus 5) that I made seems to question the fact of returning success and *vals = NULL, as another caller currently understands it as "no filter" and will copy all rows instead of none. Someone needs to think about what we want it to mean.

Details

Reviewed PR 7835 (lib/vector: Fix null pointer dereference for an empty cat_list, head 6c7d2e8) — the null-deref guard itself is correct (n_cats only grows by n >= 1, so n_cats == 0 is exactly the case where cats is still NULL, and the guard returns before both qsort(NULL, 0, ...) and cats[0]). The cppcheck suppression removal and the new pytest/conftest files follow the existing lib/gis/tests and lib/raster/tests patterns; the subprocess-with-env approach and the ctypes byref usage are sound, and count is safe to read even on the -1 path since *nvals is zeroed up front.

Findings:

• lib/vector/Vlib/cats.c:518 — Returning success with *vals = NULL makes the only in-tree caller silently copy the whole attribute table: Vect_copy_table_by_cat_list → Vect_copy_table_by_cats does if (cats) key = Fi->key; else key = NULL; (copy.c:781), and a NULL selcol in db_copy_table_by_ints means "no filter", so an empty selection copies every row instead of none. That is the exact public-API path the PR cites as motivation, so the crash becomes a silent wrong result. Note db_copy_table_by_ints treats nvals == 0 with a non-NULL selcol as DB_FAILED, i.e. the surrounding code's own convention for an empty selection is an error. Fix options: return -1, allocate a zero-length array so if (cats) stays true, or make copy.c distinguish "no list" from "empty list".
• lib/vector/Vlib/cats.c:479 — The Doxygen block still promises an allocated array on success ("Allocated array should be freed by G_free()", "return 0 on success") and does not mention that *vals can now be NULL with *nvals == 0, so external callers coded to the documented contract can dereference NULL.

[Pranav-error]

Thanks — that review is right, and I checked each step of it against the tree rather than taking it on trust. All four links hold:

• Vect_copy_table_by_cats does if (cats) key = Fi->key; else key = NULL; (copy.c:783) and passes key as selcol.
• db_copy_table_by_ints guards the whole filter with if (selcol) (copy_tab.c:87), so a null column skips filtering entirely and copies every row.
• That same block returns DB_FAILED for nvals == 0 with a non-null column — so the surrounding code already treats an empty selection as an error.
• The Doxygen promised an allocated array and said nothing about a null one.

So my patch turned a crash into a silent wrong result on exactly the path I cited as motivation, which is worse. I have changed it to return -1 for a list with no ranges.

Of the three options you listed I went with the first. The zero-length allocation would work but leans on G_malloc(0) returning non-null to keep if (cats) true, which is a fragile thing to encode a contract in. Teaching copy.c to distinguish "no list" from "empty list" is the more thorough fix, but it changes the meaning of a public function's arguments and felt like more than this bug should carry — happy to do it that way instead if you would rather.

Returning -1 also needs nothing from the caller: Vect_copy_table_by_cat_list already does if (Vect_cat_list_to_array(...) != 0) return -1;.

I updated the Doxygen to say an empty list is a failure and why, and flipped the test to assert -1. The ranges case is unchanged and still covers sorting and dedup.