From 0e74b25f626a33554feb7fb192b95ed99f807b00 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Fri, 19 Sep 2025 21:46:53 +0200 Subject: [PATCH] C API: Fix bounds checking in _byidx functions The docs weren't 100% clear about bounds checking, but suggested that errors would be caught. The bounds checks are cheap compared to the function calls they're in, so we have no reason to omit them. --- src/libexpr-c/nix_api_value.cc | 12 +++++++ src/libexpr-tests/nix_api_value.cc | 55 ++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/src/libexpr-c/nix_api_value.cc b/src/libexpr-c/nix_api_value.cc index 3339790f4..3442bf1a1 100644 --- a/src/libexpr-c/nix_api_value.cc +++ b/src/libexpr-c/nix_api_value.cc @@ -326,6 +326,10 @@ nix_value * nix_get_list_byidx(nix_c_context * context, const nix_value * value, try { auto & v = check_value_in(value); assert(v.type() == nix::nList); + if (ix >= v.listSize()) { + nix_set_err_msg(context, NIX_ERR_KEY, "list index out of bounds"); + return nullptr; + } auto * p = v.listView()[ix]; nix_gc_incref(nullptr, p); if (p != nullptr) @@ -389,6 +393,10 @@ nix_get_attr_byidx(nix_c_context * context, nix_value * value, EvalState * state try { auto & v = check_value_in(value); collapse_attrset_layer_chain_if_needed(v, state); + if (i >= v.attrs()->size()) { + nix_set_err_msg(context, NIX_ERR_KEY, "attribute index out of bounds"); + return nullptr; + } const nix::Attr & a = (*v.attrs())[i]; *name = state->state.symbols[a.name].c_str(); nix_gc_incref(nullptr, a.value); @@ -405,6 +413,10 @@ const char * nix_get_attr_name_byidx(nix_c_context * context, nix_value * value, try { auto & v = check_value_in(value); collapse_attrset_layer_chain_if_needed(v, state); + if (i >= v.attrs()->size()) { + nix_set_err_msg(context, NIX_ERR_KEY, "attribute index out of bounds (Nix C API contract violation)"); + return nullptr; + } const nix::Attr & a = (*v.attrs())[i]; return state->state.symbols[a.name].c_str(); } diff --git a/src/libexpr-tests/nix_api_value.cc b/src/libexpr-tests/nix_api_value.cc index af95224de..c74c3258f 100644 --- a/src/libexpr-tests/nix_api_value.cc +++ b/src/libexpr-tests/nix_api_value.cc @@ -162,6 +162,29 @@ TEST_F(nix_api_expr_test, nix_build_and_init_list) nix_gc_decref(ctx, intValue); } +TEST_F(nix_api_expr_test, nix_get_list_byidx_large_indices) +{ + // Create a small list to test extremely large out-of-bounds access + ListBuilder * builder = nix_make_list_builder(ctx, state, 2); + nix_value * intValue = nix_alloc_value(ctx, state); + nix_init_int(ctx, intValue, 42); + nix_list_builder_insert(ctx, builder, 0, intValue); + nix_list_builder_insert(ctx, builder, 1, intValue); + nix_make_list(ctx, builder, value); + nix_list_builder_free(builder); + + // Test extremely large indices that would definitely crash without bounds checking + ASSERT_EQ(nullptr, nix_get_list_byidx(ctx, value, state, 1000000)); + ASSERT_EQ(NIX_ERR_KEY, nix_err_code(ctx)); + ASSERT_EQ(nullptr, nix_get_list_byidx(ctx, value, state, UINT_MAX / 2)); + ASSERT_EQ(NIX_ERR_KEY, nix_err_code(ctx)); + ASSERT_EQ(nullptr, nix_get_list_byidx(ctx, value, state, UINT_MAX / 2 + 1000000)); + ASSERT_EQ(NIX_ERR_KEY, nix_err_code(ctx)); + + // Clean up + nix_gc_decref(ctx, intValue); +} + TEST_F(nix_api_expr_test, nix_build_and_init_attr_invalid) { ASSERT_EQ(nullptr, nix_get_attr_byname(ctx, nullptr, state, 0)); @@ -244,6 +267,38 @@ TEST_F(nix_api_expr_test, nix_build_and_init_attr) free(out_name); } +TEST_F(nix_api_expr_test, nix_get_attr_byidx_large_indices) +{ + // Create a small attribute set to test extremely large out-of-bounds access + const char ** out_name = (const char **) malloc(sizeof(char *)); + BindingsBuilder * builder = nix_make_bindings_builder(ctx, state, 2); + nix_value * intValue = nix_alloc_value(ctx, state); + nix_init_int(ctx, intValue, 42); + nix_bindings_builder_insert(ctx, builder, "test", intValue); + nix_make_attrs(ctx, value, builder); + nix_bindings_builder_free(builder); + + // Test extremely large indices that would definitely crash without bounds checking + ASSERT_EQ(nullptr, nix_get_attr_byidx(ctx, value, state, 1000000, out_name)); + ASSERT_EQ(NIX_ERR_KEY, nix_err_code(ctx)); + ASSERT_EQ(nullptr, nix_get_attr_byidx(ctx, value, state, UINT_MAX / 2, out_name)); + ASSERT_EQ(NIX_ERR_KEY, nix_err_code(ctx)); + ASSERT_EQ(nullptr, nix_get_attr_byidx(ctx, value, state, UINT_MAX / 2 + 1000000, out_name)); + ASSERT_EQ(NIX_ERR_KEY, nix_err_code(ctx)); + + // Test nix_get_attr_name_byidx with large indices too + ASSERT_EQ(nullptr, nix_get_attr_name_byidx(ctx, value, state, 1000000)); + ASSERT_EQ(NIX_ERR_KEY, nix_err_code(ctx)); + ASSERT_EQ(nullptr, nix_get_attr_name_byidx(ctx, value, state, UINT_MAX / 2)); + ASSERT_EQ(NIX_ERR_KEY, nix_err_code(ctx)); + ASSERT_EQ(nullptr, nix_get_attr_name_byidx(ctx, value, state, UINT_MAX / 2 + 1000000)); + ASSERT_EQ(NIX_ERR_KEY, nix_err_code(ctx)); + + // Clean up + nix_gc_decref(ctx, intValue); + free(out_name); +} + TEST_F(nix_api_expr_test, nix_value_init) { // Setup