[CLIENT-2093] Add more input validation in client config and partition filter dictionaries #582

juliannguyen4 · 2024-02-16T22:30:56Z

Valgrind

This branch: https://github.com/aerospike/aerospike-client-python/actions/runs/8104278566/job/22150570588
Dev: https://github.com/aerospike/aerospike-client-python/actions/runs/8103677912/job/22148781775

Changes

In client config, instead of silently ignoring values with invalid types, throw a ParamError
Throw a ParamError if client config option is specified as a 32-bit unsigned integer and the passed value is not a valid value of that type

Extra changes:

Fixed a bug where the client only takes in a str instead of a bytearray for the partition filter's digest at partition_filter["digest"]["value"]
If a host tuple's port is not an integer, throw a ParamError instead of setting it to 0.

Notes

Besides the changes listed above, I tried to avoid regressions wherever possible. If there was existing input validation for the setting, I didn't modify the code such that it returns a different error message.
No input validation added for deprecated / undocumented config options

codecov-commenter · 2024-02-16T23:18:11Z

Codecov Report

Attention: Patch coverage is 98.60465% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 81.84%. Comparing base (c5438a5) to head (fc8ccd5).
Report is 4 commits behind head on dev.

Files	Patch %	Lines
src/main/tls_config.c	91.42%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #582      +/-   ##
==========================================
+ Coverage   81.35%   81.84%   +0.49%     
==========================================
  Files         100      100              
  Lines       15356    15512     +156     
==========================================
+ Hits        12493    12696     +203     
+ Misses       2863     2816      -47

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…er dictionary

dwelch-spike

Currently at line 800 of type.c. I will come back to this, here are comments for now.

dwelch-spike · 2024-03-06T00:30:05Z

src/main/client/type.c

+    snprintf(error_msg, MAX_ERR_MSG_SIZE, "config%s must be a %s", config_keys,
+             expected_type_name);


Suggested change

snprintf(error_msg, MAX_ERR_MSG_SIZE, "config%s must be a %s", config_keys,

expected_type_name);

snprintf(error_msg, MAX_ERR_MSG_SIZE, "config %s must be a %s", config_keys,

expected_type_name);

spacing between "config" and the config name

dwelch-spike · 2024-03-06T00:45:56Z

src/main/client/type.c

                py_port = PyTuple_GetItem(py_host, 1);
-                if (PyLong_Check(py_port)) {
+                if (PyLong_CheckExact(py_port)) {


Why the exact check? Would a subclass cause a problem?

It's because Python booleans are subtypes of Python integers. I found this out when testing my changes
https://docs.python.org/3/c-api/bool.html

Booleans in Python are implemented as a subclass of integers

dwelch-spike · 2024-03-06T00:47:10Z

src/main/client/type.c

+                    // ["hosts"][<10 chars max>][0]
+                    // 1234567890123456789012345678
+                    // 0        10        20    + 1 = 29
+                    char keys_str[29];
+                    snprintf(keys_str, 29, "[\"hosts\"][%d][0]", i);
+                    initialize_config_value_type_err(
+                        config_value_type_error_msg, keys_str, "str",
+                        &error_code);
+                    goto CONSTRUCTOR_ERROR;
+                }
+
                py_port = PyTuple_GetItem(py_host, 1);
-                if (PyLong_Check(py_port)) {
+                if (PyLong_CheckExact(py_port)) {
                    port = (uint16_t)PyLong_AsLong(py_port);
                }
                else {
-                    port = 0;
+                    char keys_str[29];
+                    snprintf(keys_str, 29, "[\"hosts\"][%d][1]", i);
+                    initialize_config_value_type_err(


I think the "29" should be a constant, you could move the explanation from line 667 to the constant deceleration and give it a little more context.

dwelch-spike · 2024-03-06T00:47:31Z

src/main/client/type.c

+                        char keys_str[29];
+                        snprintf(keys_str, 29, "[\"hosts\"][%d][2]", i);


same here, should be a constant

dwelch-spike

At tls_config.c line 33, will come back to this

dwelch-spike · 2024-03-06T18:00:32Z

src/main/client/type.c

@@ -1152,7 +1385,7 @@ static int set_rack_aware_config(as_config *conf, PyObject *config_dict)

    py_config_value = PyDict_GetItemString(config_dict, "rack_id");
    if (py_config_value) {
-        if (PyLong_Check(py_config_value)) {
+        if (PyLong_CheckExact(py_config_value)) {


Why the exact check? I'm not sure but I don't think a sub class would cause issues.

dwelch-spike · 2024-03-06T18:00:48Z

src/main/client/type.c

@@ -1188,7 +1421,7 @@ static int set_rack_aware_config(as_config *conf, PyObject *config_dict)
        }

        Py_INCREF(rack_id_pyobj);
-        if (PyLong_Check(rack_id_pyobj) == false) {
+        if (PyLong_CheckExact(rack_id_pyobj) == false) {


Why the exact check? (same as above)

dwelch-spike · 2024-03-06T18:04:37Z

src/main/policy_config.c

@@ -768,7 +768,7 @@ as_status get_uint32_value(PyObject *py_policy_val, uint32_t *return_uint32)
    if (!py_policy_val) {
        return AEROSPIKE_ERR_PARAM;
    }
-    if (PyLong_Check(py_policy_val)) {
+    if (PyLong_CheckExact(py_policy_val)) {


same as others, why exact?

dwelch-spike · 2024-03-06T18:05:39Z

src/main/tls_config.c

@@ -25,68 +25,59 @@ static char *get_string_from_string_like(PyObject *string_like);
 *	Param: tls_conf PyDict.
 *	Fill in the appropriate TLS values of config based on the contents of
 *	tls_config
+    Returns NULL if no error occurred, or the config key and expected value type where the error occurred


nit: should start with "*" like the other lines

juliannguyen4 force-pushed the CLIENT-2093-improve-input-validation branch 2 times, most recently from 35bc608 to 42ea3de Compare February 23, 2024 00:27

juliannguyen4 force-pushed the CLIENT-2093-improve-input-validation branch 2 times, most recently from b7e566b to 8465fc2 Compare February 27, 2024 23:34

Add missing input validation for the client config and partition filt…

c012182

…er dictionary

juliannguyen4 force-pushed the CLIENT-2093-improve-input-validation branch from b696951 to c012182 Compare February 28, 2024 22:35

juliannguyen4 changed the title ~~[CLIENT-2093] Improve input validation in API~~ [CLIENT-2093] Add more input validation in client config and partition filter dictionaries Feb 28, 2024

juliannguyen4 added 7 commits February 28, 2024 15:14

Don't validate input for deprecated / undocumented config options

0e7d4f7

Check for 32-bit ints

c213e82

Update docs; remove test case bc max_threads is deprecated

ad247db

fix max_socket_idle case

4e75b70

Fix test

2703585

Fix bug

58f07c2

Fix memory leaks

b9523ab

juliannguyen4 requested a review from dwelch-spike March 1, 2024 16:32

juliannguyen4 marked this pull request as ready for review March 1, 2024 16:32

dwelch-spike requested changes Mar 6, 2024

View reviewed changes

dwelch-spike reviewed Mar 6, 2024

View reviewed changes

juliannguyen4 added 2 commits May 13, 2024 09:12

Merge branch 'dev' into CLIENT-2093-improve-input-validation

d9c5793

Add more test coverage

fc8ccd5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CLIENT-2093] Add more input validation in client config and partition filter dictionaries #582

[CLIENT-2093] Add more input validation in client config and partition filter dictionaries #582

juliannguyen4 commented Feb 16, 2024 •

edited

codecov-commenter commented Feb 16, 2024 •

edited

dwelch-spike left a comment

dwelch-spike Mar 6, 2024

dwelch-spike Mar 6, 2024

juliannguyen4 Mar 8, 2024

dwelch-spike Mar 6, 2024

dwelch-spike Mar 6, 2024

dwelch-spike left a comment

dwelch-spike Mar 6, 2024

dwelch-spike Mar 6, 2024

dwelch-spike Mar 6, 2024

dwelch-spike Mar 6, 2024

		snprintf(error_msg, MAX_ERR_MSG_SIZE, "config%s must be a %s", config_keys,
		expected_type_name);

		char keys_str[29];
		snprintf(keys_str, 29, "[\"hosts\"][%d][2]", i);

[CLIENT-2093] Add more input validation in client config and partition filter dictionaries #582

Are you sure you want to change the base?

[CLIENT-2093] Add more input validation in client config and partition filter dictionaries #582

Conversation

juliannguyen4 commented Feb 16, 2024 • edited

codecov-commenter commented Feb 16, 2024 • edited

Codecov Report

dwelch-spike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwelch-spike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliannguyen4 commented Feb 16, 2024 •

edited

codecov-commenter commented Feb 16, 2024 •

edited