diff --git a/src/include/cdt_operation_utils.h b/src/include/cdt_operation_utils.h index 65b0d27c6a..fed2c249fe 100644 --- a/src/include/cdt_operation_utils.h +++ b/src/include/cdt_operation_utils.h @@ -74,8 +74,18 @@ as_status get_optional_int64_t(as_error *err, const char *key, as_status get_uint64_t(as_error *err, const char *key, PyObject *op_dict, uint64_t *ui64_valptr); +// This is used to validate enum arguments +// In C99, enum values can be between INT_MIN and INT_MAX +// So we define our min and max bound parameters as integer types +// https://stackoverflow.com/a/366033 +// If is_optional is true, int_pointer does not get dereferenced. +// min_bound and max_bound are inclusive +as_status get_enum_from_py_dict(as_error *err, const char *key, + PyObject *py_dict, int *int_pointer, + int min_bound, int max_bound, bool is_optional); + as_status get_int_from_py_dict(as_error *err, const char *key, - PyObject *op_dict, int *int_pointer); + PyObject *py_dict, int *int_pointer); as_status get_list_return_type(as_error *err, PyObject *op_dict, int *return_type); diff --git a/src/main/client/bit_operate.c b/src/main/client/bit_operate.c index 5785818419..1d115592cb 100644 --- a/src/main/client/bit_operate.c +++ b/src/main/client/bit_operate.c @@ -343,17 +343,15 @@ as_status add_new_bit_op(AerospikeClient *self, as_error *err, static as_status get_bit_resize_flags(as_error *err, PyObject *op_dict, as_bit_resize_flags *resize_flags) { - int64_t flags64; - bool found = false; + int tmp_value; *resize_flags = AS_BIT_RESIZE_DEFAULT; - if (get_optional_int64_t(err, RESIZE_FLAGS_KEY, op_dict, &flags64, - &found) != AEROSPIKE_OK) { + if (get_enum_from_py_dict( + err, RESIZE_FLAGS_KEY, op_dict, &tmp_value, AS_BIT_RESIZE_DEFAULT, + AS_BIT_RESIZE_SHRINK_ONLY * 2 - 1, true) != AEROSPIKE_OK) { return err->code; } - if (found) { - *resize_flags = (as_bit_resize_flags)flags64; - } + *resize_flags = (as_bit_resize_flags)tmp_value; return AEROSPIKE_OK; } diff --git a/src/main/client/cdt_operation_utils.c b/src/main/client/cdt_operation_utils.c index fce6e95892..c733961713 100644 --- a/src/main/client/cdt_operation_utils.c +++ b/src/main/client/cdt_operation_utils.c @@ -196,22 +196,67 @@ as_status get_uint64_t(as_error *err, const char *key, PyObject *op_dict, return AEROSPIKE_OK; } -as_status get_int_from_py_dict(as_error *err, const char *key, - PyObject *op_dict, int *int_pointer) +#define OUT_OF_BOUNDS_MESSAGE \ + "%s must be inclusively between %d and %d, but received %" PRId64 \ + " instead." + +static inline as_status +get_bound_int_from_py_dict(as_error *err, const char *key, PyObject *py_dict, + int *int_pointer, int min_bound, int max_bound, + bool is_optional, bool warn_if_out_of_bounds) { - int64_t int64_to_return = -1; + int64_t int64 = -1; + bool found = false; + if (get_optional_int64_t(err, key, py_dict, &int64, &found) != + AEROSPIKE_OK) { + goto exit_without_returning_int; + } - if (get_int64_t(err, key, op_dict, &int64_to_return) != AEROSPIKE_OK) { - return err->code; + if (!found) { + if (!is_optional) { + return as_error_update(err, AEROSPIKE_ERR_PARAM, + "Operation missing required entry %s", key); + } + else { + goto exit_without_returning_int; + } } - if (int64_to_return > INT_MAX || int64_to_return < INT_MIN) { - return as_error_update(err, AEROSPIKE_ERR_PARAM, - "%s too large for C int.", key); + if (int64 >= min_bound && int64 <= max_bound) { + goto return_int; } - *int_pointer = int64_to_return; - return AEROSPIKE_OK; + if (warn_if_out_of_bounds) { + int retval = PyErr_WarnFormat(PyExc_DeprecationWarning, STACK_LEVEL, + OUT_OF_BOUNDS_MESSAGE, key, min_bound, + max_bound, int64); + if (retval == 0) { + goto return_int; + } + } + + return as_error_update(err, AEROSPIKE_ERR_PARAM, OUT_OF_BOUNDS_MESSAGE, key, + min_bound, max_bound, int64); + +return_int: + *int_pointer = int64; +exit_without_returning_int: + return err->code; +} + +as_status get_enum_from_py_dict(as_error *err, const char *key, + PyObject *py_dict, int *int_pointer, + int min_bound, int max_bound, bool is_optional) +{ + return get_bound_int_from_py_dict(err, key, py_dict, int_pointer, min_bound, + max_bound, is_optional, true); +} + +as_status get_int_from_py_dict(as_error *err, const char *key, + PyObject *py_dict, int *int_pointer) +{ + return get_bound_int_from_py_dict(err, key, py_dict, int_pointer, INT_MIN, + INT_MAX, false, false); } as_status get_list_return_type(as_error *err, PyObject *op_dict, diff --git a/src/main/client/operate.c b/src/main/client/operate.c index b85bd6619e..a826e1b6a8 100644 --- a/src/main/client/operate.c +++ b/src/main/client/operate.c @@ -37,6 +37,7 @@ #include "hll_operations.h" #include "pythoncapi_compat.h" #include "expression_operations.h" +#include "cdt_operation_utils.h" #include #include @@ -346,7 +347,6 @@ as_status add_op(AerospikeClient *self, as_error *err, PyObject *py_map_policy = NULL; PyObject *py_return_type = NULL; // For map_create operation - PyObject *py_map_order = NULL; PyObject *py_persist_index = NULL; Py_ssize_t pos = 0; @@ -419,7 +419,7 @@ as_status add_op(AerospikeClient *self, as_error *err, ctx_ref = (ctx_in_use ? &ctx : NULL); } else if (strcmp(name, "map_order") == 0) { - py_map_order = value; + continue; } else if (strcmp(name, "persist_index") == 0) { py_persist_index = value; @@ -744,7 +744,15 @@ as_status add_op(AerospikeClient *self, as_error *err, as_operations_map_set_policy(ops, bin, ctx_ref, &map_policy); break; case OP_MAP_CREATE:; - as_map_order order = (as_map_order)PyLong_AsLong(py_map_order); + int tmp_value; + if (get_enum_from_py_dict(err, "map_order", py_operation_dict, + &tmp_value, AS_MAP_UNORDERED, + AS_MAP_KEY_VALUE_ORDERED, + false) != AEROSPIKE_OK) { + return err->code; + } + as_map_order order = (as_map_order)tmp_value; + bool persist_index = PyObject_IsTrue(py_persist_index); as_operations_map_create_all(ops, bin, ctx_ref, order, persist_index); break; diff --git a/src/main/client/operate_helper.c b/src/main/client/operate_helper.c index f641d933e5..33ba5e390d 100644 --- a/src/main/client/operate_helper.c +++ b/src/main/client/operate_helper.c @@ -191,14 +191,17 @@ as_status as_operations_add_from_pyobject(AerospikeClient *self, as_error *err, break; } - int64_t order_type_int; + as_list_order list_order = AS_LIST_UNORDERED; + int tmp_value; switch (operation_code) { case OP_LIST_SET_ORDER: case OP_LIST_CREATE: - if (get_int64_t(err, AS_PY_LIST_ORDER, op_dict, &order_type_int) != - AEROSPIKE_OK) { + if (get_enum_from_py_dict(err, AS_PY_LIST_ORDER, op_dict, &tmp_value, + AS_LIST_UNORDERED, AS_LIST_ORDERED, + false) != AEROSPIKE_OK) { goto exit; } + list_order = (as_list_order)tmp_value; } bool ctx_in_use = false; @@ -330,11 +333,11 @@ as_status as_operations_add_from_pyobject(AerospikeClient *self, as_error *err, as_string_numeric_type numeric_type = AS_STRING_NUMERIC_ANY; as_string_regex_flags regex_flags = AS_STRING_REGEX_FLAGS_NONE; - int64_t tmp_value; switch (operation_code) { case OP_STRING_IS_NUMERIC: { - if (get_int64_t(err, "numeric_type", op_dict, &tmp_value) != - AEROSPIKE_OK) { + if (get_enum_from_py_dict( + err, "numeric_type", op_dict, &tmp_value, AS_STRING_NUMERIC_ANY, + AS_STRING_NUMERIC_FLOAT, false) != AEROSPIKE_OK) { goto CLEANUP_VAL2_ON_ERROR; } numeric_type = (as_string_numeric_type)tmp_value; @@ -342,8 +345,10 @@ as_status as_operations_add_from_pyobject(AerospikeClient *self, as_error *err, } case OP_STRING_REGEX_COMPARE: case OP_STRING_REGEX_REPLACE: { - if (get_int64_t(err, "regex_flags", op_dict, &tmp_value) != - AEROSPIKE_OK) { + if (get_enum_from_py_dict(err, "regex_flags", op_dict, &tmp_value, + AS_STRING_REGEX_FLAGS_NONE, + AS_STRING_REGEX_FLAGS_GLOBAL * 2 - 1, + false) != AEROSPIKE_OK) { goto CLEANUP_VAL2_ON_ERROR; } regex_flags = (as_string_regex_flags)tmp_value; @@ -557,18 +562,19 @@ as_status as_operations_add_from_pyobject(AerospikeClient *self, as_error *err, ops, bin, ctx_ref, val1, val2, return_type); break; case OP_LIST_SET_ORDER: - success = as_operations_list_set_order(ops, bin, ctx_ref, - (as_list_order)order_type_int); + success = as_operations_list_set_order(ops, bin, ctx_ref, list_order); break; case OP_LIST_SORT: { - int64_t sort_flags; + as_list_sort_flags sort_flags; - if (get_int64_t(err, AS_PY_LIST_SORT_FLAGS, op_dict, &sort_flags) != - AEROSPIKE_OK) { + if (get_enum_from_py_dict(err, AS_PY_LIST_SORT_FLAGS, op_dict, + &tmp_value, AS_LIST_SORT_DEFAULT, + AS_LIST_SORT_DROP_DUPLICATES, + false) != AEROSPIKE_OK) { goto CLEANUP_VAL2_ON_ERROR; } - success = as_operations_list_sort(ops, bin, ctx_ref, - (as_list_sort_flags)sort_flags); + sort_flags = (as_list_sort_flags)tmp_value; + success = as_operations_list_sort(ops, bin, ctx_ref, sort_flags); break; } case OP_LIST_GET_BY_VALUE_RANK_RANGE_REL: @@ -593,8 +599,7 @@ as_status as_operations_add_from_pyobject(AerospikeClient *self, as_error *err, goto CLEANUP_VAL2_ON_ERROR; } - success = as_operations_list_create_all(ops, bin, ctx_ref, - (as_list_order)order_type_int, + success = as_operations_list_create_all(ops, bin, ctx_ref, list_order, pad, persist_index); break; } diff --git a/src/main/conversions.c b/src/main/conversions.c index 6e6e06ca99..e7dc02fd4f 100644 --- a/src/main/conversions.c +++ b/src/main/conversions.c @@ -2640,8 +2640,9 @@ as_status as_cdt_ctx_add_from_pyobject(AerospikeClient *self, as_error *err, break; case CDT_CTX_MAP_KEY_CREATE:; int map_order = 0; - status = get_int_from_py_dict(err, CDT_CTX_ORDER_KEY, py_extra_args, - &map_order); + status = get_enum_from_py_dict(err, CDT_CTX_ORDER_KEY, py_extra_args, + &map_order, AS_MAP_UNORDERED, + AS_MAP_KEY_VALUE_ORDERED, false); if (status != AEROSPIKE_OK) { goto CLEANUP_PY_EXTRA_ARGS; } @@ -2664,8 +2665,9 @@ as_status as_cdt_ctx_add_from_pyobject(AerospikeClient *self, as_error *err, case CDT_CTX_LIST_INDEX_CREATE:; int list_order = 0; int pad = 0; - status = get_int_from_py_dict(err, CDT_CTX_ORDER_KEY, py_extra_args, - &list_order); + status = get_enum_from_py_dict(err, CDT_CTX_ORDER_KEY, py_extra_args, + &list_order, AS_LIST_UNORDERED, + AS_LIST_ORDERED, false); if (status != AEROSPIKE_OK) { goto CLEANUP_PY_EXTRA_ARGS; } diff --git a/src/main/convert_expressions.c b/src/main/convert_expressions.c index 133ef9e84a..41874cd763 100644 --- a/src/main/convert_expressions.c +++ b/src/main/convert_expressions.c @@ -1048,15 +1048,18 @@ add_expr_macros(AerospikeClient *self, as_static_pool *static_pool, APPEND_ARRAY(1, as_exp_list_clear(temp_expr->ctx, NIL)); // -1 for bin break; - case OP_LIST_SORT: - if (get_int64_t(err, LIST_ORDER_KEY, temp_expr->pydict, &lval1) != - AEROSPIKE_OK) { + case OP_LIST_SORT: { + int tmp_value; + if (get_enum_from_py_dict(err, LIST_ORDER_KEY, temp_expr->pydict, + &tmp_value, AS_LIST_UNORDERED, + AS_LIST_ORDERED, false) != AEROSPIKE_OK) { return err->code; } - APPEND_ARRAY(1, as_exp_list_sort(temp_expr->ctx, lval1, + APPEND_ARRAY(1, as_exp_list_sort(temp_expr->ctx, tmp_value, NIL)); // -1 for bin break; + } case OP_LIST_REMOVE_BY_VALUE: if (get_int64_t(err, AS_PY_LIST_RETURN_KEY, temp_expr->pydict, &lval1) != AEROSPIKE_OK) { @@ -1482,14 +1485,18 @@ add_expr_macros(AerospikeClient *self, as_static_pool *static_pool, temp_expr->ctx, lval1, NIL, NIL, NIL)); // - 3 for rank, count, bin break; - case _AS_EXP_BIT_FLAGS: - if (get_int64_t(err, AS_PY_VAL_KEY, temp_expr->pydict, &lval1) != - AEROSPIKE_OK) { + case _AS_EXP_BIT_FLAGS: { + int tmp_value; + if (get_enum_from_py_dict(err, AS_PY_VAL_KEY, temp_expr->pydict, + &tmp_value, AS_BIT_RESIZE_DEFAULT, + AS_BIT_RESIZE_SHRINK_ONLY * 2 - 1, + false) != AEROSPIKE_OK) { return err->code; } - APPEND_ARRAY(0, as_exp_uint((uint64_t)lval1)); + APPEND_ARRAY(0, as_exp_uint(tmp_value)); break; + } case OP_BIT_RESIZE: APPEND_ARRAY(4, as_exp_bit_resize( NULL, NIL, NO_BIT_FLAGS, diff --git a/src/main/policy.c b/src/main/policy.c index 231cca2890..910388229b 100644 --- a/src/main/policy.c +++ b/src/main/policy.c @@ -35,6 +35,7 @@ #include "policy.h" #include "macros.h" #include "policy_config.h" +#include "cdt_operation_utils.h" #define MAP_WRITE_FLAGS_KEY "map_write_flags" #define BIT_WRITE_FLAGS_KEY "bit_write_flags" @@ -1043,22 +1044,17 @@ as_status pyobject_to_bit_policy(as_error *err, PyObject *py_policy, } } - PyObject *py_bit_flags = - PyDict_GetItemString(py_policy, BIT_WRITE_FLAGS_KEY); - if (py_bit_flags) { - if (PyLong_Check(py_bit_flags)) { - as_bit_write_flags bit_write_flags = - (as_bit_write_flags)PyLong_AsLong(py_bit_flags); - as_bit_policy_set_write_flags(policy, bit_write_flags); - } - } - else if (PyErr_Occurred()) { - /* Fetching a map key failed internally for some reason, raise an error and exit.*/ - PyErr_Clear(); - return as_error_update(err, AEROSPIKE_ERR_CLIENT, - "Unable to get bit_write_flags"); + int tmp_value; + if (get_enum_from_py_dict(err, BIT_WRITE_FLAGS_KEY, py_policy, &tmp_value, + AS_BIT_WRITE_DEFAULT, + AS_BIT_WRITE_PARTIAL * 2 - 1, + true) != AEROSPIKE_OK) { + return err->code; } + as_bit_write_flags bit_write_flags = (as_bit_write_flags)tmp_value; + as_bit_policy_set_write_flags(policy, bit_write_flags); + return err->code; } as_status pyobject_to_map_policy(as_error *err, PyObject *py_policy, @@ -1081,11 +1077,18 @@ as_status pyobject_to_map_policy(as_error *err, PyObject *py_policy, } // Defaults - long map_order = AS_MAP_UNORDERED; uint32_t map_write_flags = AS_MAP_WRITE_DEFAULT; bool persist_index = false; - MAP_POLICY_SET_FIELD(map_order, PyLong_AsLong); + as_map_order map_order = AS_MAP_UNORDERED; + int tmp_value; + if (get_enum_from_py_dict(err, "map_order", py_policy, &tmp_value, + AS_MAP_UNORDERED, AS_MAP_KEY_VALUE_ORDERED, + true) != AEROSPIKE_OK) { + return err->code; + } + map_order = (as_map_order)tmp_value; + MAP_POLICY_SET_FIELD(map_write_flags, PyLong_AsUnsignedLong); PyObject *py_persist_index = @@ -1111,9 +1114,6 @@ as_status pyobject_to_list_policy(as_error *err, PyObject *py_policy, bool validate_keys) { as_list_policy_init(list_policy); - PyObject *py_val = NULL; - long list_order = AS_LIST_UNORDERED; - long flags = AS_LIST_WRITE_DEFAULT; if (!py_policy || py_policy == Py_None) { return AEROSPIKE_OK; @@ -1137,38 +1137,25 @@ as_status pyobject_to_list_policy(as_error *err, PyObject *py_policy, } } - py_val = PyDict_GetItemString(py_policy, "list_order"); - if (py_val && py_val != Py_None) { - if (PyLong_Check(py_val)) { - list_order = PyLong_AsLong(py_val); - if (PyErr_Occurred()) { - return as_error_update(err, AEROSPIKE_ERR_PARAM, - "Failed to convert list_order"); - } - } - else { - return as_error_update(err, AEROSPIKE_ERR_PARAM, - "Invalid List order"); - } + int tmp_value; + as_list_order list_order = AS_LIST_UNORDERED; + + if (get_enum_from_py_dict(err, "list_order", py_policy, &tmp_value, + AS_LIST_UNORDERED, AS_LIST_ORDERED, + true) != AEROSPIKE_OK) { + return err->code; } + list_order = (as_list_order)tmp_value; - py_val = PyDict_GetItemString(py_policy, "write_flags"); - if (py_val && py_val != Py_None) { - if (PyLong_Check(py_val)) { - flags = PyLong_AsLong(py_val); - if (PyErr_Occurred()) { - return as_error_update(err, AEROSPIKE_ERR_PARAM, - "Failed to convert write_flags"); - } - } - else { - return as_error_update(err, AEROSPIKE_ERR_PARAM, - "Invalid write_flags"); - } + as_list_write_flags flags = AS_LIST_WRITE_DEFAULT; + if (get_enum_from_py_dict( + err, "write_flags", py_policy, &tmp_value, AS_LIST_WRITE_DEFAULT, + AS_LIST_WRITE_PARTIAL * 2 - 1, true) != AEROSPIKE_OK) { + return err->code; } + flags = (as_list_write_flags)tmp_value; - as_list_policy_set(list_policy, (as_list_order)list_order, - (as_list_write_flags)flags); + as_list_policy_set(list_policy, list_order, flags); return AEROSPIKE_OK; } @@ -1176,9 +1163,7 @@ as_status pyobject_to_list_policy(as_error *err, PyObject *py_policy, as_status pyobject_to_hll_policy(as_error *err, PyObject *py_policy, as_hll_policy *hll_policy, bool validate_keys) { - int64_t flags = 0; as_hll_policy_init(hll_policy); - PyObject *py_val = NULL; if (!py_policy || py_policy == Py_None) { return AEROSPIKE_OK; @@ -1202,21 +1187,15 @@ as_status pyobject_to_hll_policy(as_error *err, PyObject *py_policy, } } - py_val = PyDict_GetItemString(py_policy, "flags"); - if (py_val && py_val != Py_None) { - if (PyLong_Check(py_val)) { - flags = (int64_t)PyLong_AsLongLong(py_val); - if (PyErr_Occurred()) { - return as_error_update(err, AEROSPIKE_ERR_PARAM, - "Failed to convert flags."); - } - } - else { - return as_error_update(err, AEROSPIKE_ERR_PARAM, - "Invalid hll policy flags."); - } + int tmp_value; + as_hll_write_flags flags; + if (get_enum_from_py_dict( + err, "flags", py_policy, &tmp_value, AS_HLL_WRITE_DEFAULT, + AS_HLL_WRITE_ALLOW_FOLD * 2 - 1, true) != AEROSPIKE_OK) { + return err->code; } + flags = (as_hll_write_flags)tmp_value; as_hll_policy_set_write_flags(hll_policy, flags); return AEROSPIKE_OK; diff --git a/test/new_tests/test_invalid_options.py b/test/new_tests/test_invalid_options.py new file mode 100644 index 0000000000..c0fab1e3ce --- /dev/null +++ b/test/new_tests/test_invalid_options.py @@ -0,0 +1,30 @@ +from .conftest import KEYS +import pytest + +from aerospike_helpers.operations import bitwise_operations, map_operations, list_operations, hll_operations +import aerospike +from aerospike import exception as e + +@pytest.mark.usefixtures("as_connection") +class TestInvalidOptions: + @pytest.mark.parametrize( + "op", + [ + bitwise_operations.bit_lshift(bin_name="bitwise", bit_offset=0, bit_size=2, shift=1, policy={"bit_write_flags": -1}), + bitwise_operations.bit_resize(bin_name="bitwise", byte_size=1, resize_flags=aerospike.BIT_RESIZE_SHRINK_ONLY * 2), + map_operations.map_put(bin_name="map", key=1, value=1, map_policy={"map_order": aerospike.MAP_KEY_VALUE_ORDERED + 1}), + list_operations.list_append(bin_name="list", value=1, policy={"list_order": aerospike.LIST_ORDERED + 1}), + list_operations.list_append(bin_name="list", value=1, policy={"write_flags": aerospike.LIST_WRITE_PARTIAL * 2}), + hll_operations.hll_add(bin_name="hll", values=[1], policy={"flags": aerospike.HLL_WRITE_ALLOW_FOLD * 2}), + ] + ) + def test_invalid_enum_values_emits_warning(self, op): + ops = [ + op + ] + try: + with pytest.warns(DeprecationWarning): + self.as_connection.operate(KEYS[0], ops) + # We only care about the client printing the DeprecationWarning; this is not an end to end test + except e.ServerError: + pass