From 8a6eef96b2c2eec524f0be36017ae38ba824990f Mon Sep 17 00:00:00 2001 From: Anton Date: Wed, 30 Sep 2026 14:07:19 +0200 Subject: [PATCH] fix: ray_execute and index attach never return NULL Running cppcheck one file at a time -- which enables the cross-file checks CI's parallel whole-tree run skips -- reported six ctunullpointer warnings. Each has the same shape: a value that RAY_IS_ERR lets through as NULL, then a dereference. Traced to the producers, two are real, reachable on allocation failure: - ray_execute: ray_execute_inner hands NULL back unchanged when a node, a compaction or a merge fails to allocate -- the flat path's final `return result` and the streaming path's seg_tbl / partial / merged returns; only the streaming tail converted NULL to "oom". Callers test RAY_IS_ERR, which is false for NULL: ray_lazy_materialize passes it on, and (times ...) dereferences it in loop_count (eval.c:1975). ray_execute now converts NULL to an "oom" error at its single exit. - prepare_attach_ex (idxop.c): a failed ray_cow, or a failed copy inside ray_index_drop, returned NULL to all five callers (.idx.hash, dict, part, built, plain attach), which test only RAY_IS_ERR and then dereference (idxop.c:711). After ray_index_drop it also stored that NULL into the caller's *vp. Both now return "oom" and leave *vp intact. One is dead code made consistent: exec_group_v2 tested !g and then passed g to exec_group_v2_run, which dereferences it (internal.h:650). Every caller passes a graph; a NULL one now returns "nyi" like ray_execute. The other three are false positives, inferred only from the NULL test inside RAY_IS_ERR: ray_try_handle's handler (eval.c:301) comes from ray_eval of a non-NULL expression; filter.c's gather only reaches `col` for a new_cols[c] built from col->type; update_eval_on already converts NULL to an error (query.c:14072). After this commit idxop.c:711 and eval.c:1975 are false positives too, their producers no longer returning NULL. No regression test: every real path needs an allocation to fail and there is no allocator fault injection to force one. make test 3956/3956. --- src/ops/agg_engine.c | 5 ++++- src/ops/exec.c | 8 +++++++- src/ops/idxop.c | 6 +++++- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/ops/agg_engine.c b/src/ops/agg_engine.c index 984faff6..f5b33887 100644 --- a/src/ops/agg_engine.c +++ b/src/ops/agg_engine.c @@ -5421,7 +5421,10 @@ ray_t* exec_group_v2(ray_graph_t* g, ray_op_t* op, ray_t* tbl, * groups themselves; the other routes trim their full result. */ ray_group_emit_filter_t ef = ray_group_emit_filter_active(); const ray_group_emit_filter_t* efp = ef.enabled ? &ef : NULL; - if (!g || !g->selection) + /* exec_group_v2_run reads g's op extensions unconditionally; a NULL + * graph is an error here, not a request for an unfiltered run. */ + if (!g) return ray_error("nyi", NULL); + if (!g->selection) return exec_group_v2_run(g, op, tbl, ray_table_nrows(tbl), NULL, NULL, 0, group_limit, efp); diff --git a/src/ops/exec.c b/src/ops/exec.c index daa2fe30..147412cf 100644 --- a/src/ops/exec.c +++ b/src/ops/exec.c @@ -4060,7 +4060,13 @@ ray_t* ray_execute(ray_graph_t* g, ray_op_t* root) { * would reset the elapsed clock and fire premature "final" ticks. */ ray_t* scan_err = validate_scan_columns(g); if (scan_err) return scan_err; - return ray_execute_inner(g, root); + /* Never NULL: callers test RAY_IS_ERR, which is false for NULL, and then + * dereference. The inner paths hand NULL back unchanged when a node, + * a compaction or a merge fails to allocate (the flat path's + * `return result`, the streaming path's seg_tbl / partial / merged + * returns); `(times ...)` would then crash in loop_count. */ + ray_t* result = ray_execute_inner(g, root); + return result ? result : ray_error("oom", NULL); } /* Flatten one parted/mapcommon column into a dense vector (mirrors the diff --git a/src/ops/idxop.c b/src/ops/idxop.c index cc9f9a36..05baa57f 100644 --- a/src/ops/idxop.c +++ b/src/ops/idxop.c @@ -745,11 +745,15 @@ static ray_t* prepare_attach_ex(ray_t** vp, const char* what, return ray_error("type", "%s: cannot index a slice; materialize first", what); if (v->attrs & RAY_ATTR_HAS_INDEX) { ray_index_drop(&v); + if (!v) return ray_error("oom", NULL); /* keep *vp intact */ if (RAY_IS_ERR(v)) return v; *vp = v; } v = ray_cow(v); - if (!v || RAY_IS_ERR(v)) return v; + /* Never hand back NULL: every caller tests only RAY_IS_ERR and then + * dereferences, so a failed copy must surface as an error. */ + if (!v) return ray_error("oom", NULL); + if (RAY_IS_ERR(v)) return v; *vp = v; /* Numeric vectors carry any index kind; STR carries only RAY_IDX_DICT * (codes live alongside the descriptors — the column representation is