Error when setting a property to an array containing an invalid type #1898

jeffreylovitz · 2021-08-26T19:15:25Z

Previously, when saving an array as a property value, we did not introspect on the arrays contents to make sure that all elements were non-NULL primitive data types or arrays thereof. As such, invalid formulations such as:

MATCH (a)-[]->(b) SET b.prop = [a]

Were accepted, and would only crash in serialization.

This PR adds recursive array-checking to make sure that only valid data types are saved within property arrays.

Resolves #1861

…type

codecov · 2021-08-26T19:34:05Z

Codecov Report

Merging #1898 (0924f0d) into master (5a5226f) will decrease coverage by 5.94%.
The diff coverage is 100.00%.

❗ Current head 0924f0d differs from pull request most recent head 801a279. Consider uploading reports for the commit 801a279 to get more accurate results

@@            Coverage Diff             @@
##           master    #1898      +/-   ##
==========================================
- Coverage   92.61%   86.66%   -5.95%     
==========================================
  Files         239      247       +8     
  Lines       20300    21588    +1288     
==========================================
- Hits        18801    18710      -91     
- Misses       1499     2878    +1379

Impacted Files	Coverage Δ
src/datatypes/array.c	`71.18% <100.00%> (+5.18%)`	⬆️
src/execution_plan/ops/shared/create_functions.c	`100.00% <100.00%> (ø)`
src/execution_plan/ops/shared/update_functions.c	`98.93% <100.00%> (+1.20%)`	⬆️
...serializers/decoders/prev/v7/decode_graph_schema.c	`0.00% <0.00%> (-100.00%)`	⬇️
...serializers/decoders/prev/v8/decode_graph_schema.c	`0.00% <0.00%> (-100.00%)`	⬇️
...rializers/decoders/prev/v7/decode_graph_entities.c	`0.00% <0.00%> (-100.00%)`	⬇️
src/serializers/decoders/prev/v8/decode_graph.c	`0.00% <0.00%> (-96.71%)`	⬇️
src/serializers/decoders/prev/v7/decode_graph.c	`0.00% <0.00%> (-96.43%)`	⬇️
...rializers/decoders/prev/v8/decode_graph_entities.c	`0.00% <0.00%> (-96.16%)`	⬇️
src/serializers/decoders/decode_previous.c	`30.76% <0.00%> (-41.96%)`	⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a5226f...801a279. Read the comment docs.

swilly22 · 2021-08-28T18:03:16Z

tests/flow/test_entity_update.py

+        # Skip this test if running under Valgrind, as it causes a memory leak.
+        if Env().envRunner.debugger is not None:
+            Env().skip()


Are we leaking here?

We had been; I rewrote the validation such that it no longer leaks.

As with many other run-time validations, it is still possible to cause leaks when jumping to the exception handler. For example, this query would leak:

WITH {k: 'v'} AS map MATCH (a) SET a.v = [map]

(In this case, map will leak due to its allocation at https://github.com/RedisGraph/RedisGraph/blob/master/src/execution_plan/ops/op_update.c#L75)

I don't believe these leaks can be entirely plugged without a change like SIValue reference counting.

swilly22 · 2021-08-28T18:04:56Z

tests/flow/test_entity_update.py

+            graph.query("MATCH (a) SET a = {v: ['str', [1, NULL]]}")
+            self.env.assertTrue(False)
+        except ResponseError as e:
+            self.env.assertContains("Property values can only be of primitive types or arrays of primitive types", str(e))


We should also test for all cases where an array contains a restricted type:

map

node

edge

null

swilly22 · 2021-08-31T17:05:02Z

src/execution_plan/ops/shared/update_functions.c

+static bool _ValidateArrayProperty(SIValue arr) {
+	ASSERT(SI_TYPE(arr) == T_ARRAY);
+
+	uint array_len = SIArray_Length(arr);
+	for(uint i = 0; i < array_len; i++) {
+		SIValue elem = SIArray_Get(arr, i);
+		// each element must be a primitive or an array
+		if(!(SI_TYPE(elem) & SI_VALID_PROPERTY_VALUE)) return false;
+
+		// recursively check nested arrays
+		if(SI_TYPE(elem) == T_ARRAY) return _ValidateArrayProperty(elem);
+	}
+
+	return true;
+}


I think it would be cleaner if we introduce bool containType(SIArray arr, SIType t) to SIArray,
this way we can call this function using bitwise or to describe all forbidden types. also it seems like we might have missed cases where we set a property to an invalid value upon entity creation.

swilly22 · 2021-09-08T14:26:44Z

src/datatypes/array.c

+		if(SI_TYPE(elem) & t) return true;
+
+		// recursively check nested arrays
+		if(SI_TYPE(elem) == T_ARRAY) {


For completeness, I think we should also recursively test Map data type
add unit-test for this function as part of test_value.cpp or introduce test_array.cpp

I don't quite follow the first part of this request. Map types cannot be used as property values, so the logic in this PR disallows them in the same way that a node or edge value would be disallowed.

Although you're looking at this from a specific context.

swilly22 · 2021-09-08T14:34:13Z

src/execution_plan/ops/shared/create_functions.c

@@ -244,6 +245,21 @@ PendingProperties *ConvertPropertyMap(Record r, PropertyMap *map, bool fail_on_n
 				ErrorCtx_RaiseRuntimeException("Cannot merge node using null property value");
 			}
 		}
+
+		// emit an error and exit if we're trying to add


I've notice converted is leaking in-case of an error, consider switching to a two loops logic:

1. validate expressions (can through an error) 2. allocate 3. assign

We can restructure the function like that, but I don't think it will fix any existing leaks. In the event of an error on property N, we call PendingPropertiesFree(converted) and free the previously computed property values 0..N.

swilly22 · 2021-09-08T14:34:39Z

src/execution_plan/ops/shared/create_functions.c

+		// emit an error and exit if we're trying to add
+		// an array containing an invalid type
+		if(SI_TYPE(val) == T_ARRAY) {
+			SIType invalid_properties = ~SI_VALID_PROPERTY_VALUE;


swilly22 · 2021-09-08T14:41:17Z

tests/flow/test_graph_create.py

@@ -102,3 +102,18 @@ def test06_create_project_volatile_value(self):

        self.env.assertEquals(result.nodes_created, 1)
        self.env.assertEquals(result.result_set, expected_result)
+
+    # Fail when a property is a complex type nested within an array type
+    def test13_invalid_complex_type_in_array(self):


rename function name, test14... or combine with test13_invalid_complex_type_in_array

swilly22 · 2021-09-08T19:15:00Z

tests/unit/test_value.cpp

+	ASSERT_TRUE(contains_double);
+
+	bool contains_string = SIArray_ContainsType(arr, T_STRING);
+	ASSERT_FALSE(contains_string);


Please add a test which checks the combination of types: SIArray_ContainsType(arr, t0&t1)

…1898) * Emit error when setting a property to an array containing an invalid type * Address PR comments * Address PR comments * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* Error when setting a property to an array containing an invalid type (#1898) * Emit error when setting a property to an array containing an invalid type * Address PR comments * Address PR comments * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Aliases in WITH...ORDER BY must be valid references (#1897) * Ensure aliases in the ORDER BY segment of a WITH clause are valid references * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Error when setting a property to an array containing an invalid type (#1898) * Emit error when setting a property to an array containing an invalid type * Address PR comments * Address PR comments * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * increase code coverage (#1919) * clean unused files * delete v5 decoder * test decoders * fix build * delete unused functions * add deleted nodes and edges to rdb * fix unused function * address review * Error when setting a property to an array containing an invalid type (#1898) * Emit error when setting a property to an array containing an invalid type * Address PR comments * Address PR comments * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Storing test artifacts for CI runs (#1906) * Storing test artifacts * PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Error when setting a property to an array containing an invalid type (#1898) * Emit error when setting a property to an array containing an invalid type * Address PR comments * Address PR comments * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * sync matrices on parent process before fork (#1931) * sync matrices on parent process before fork * formatting * Error when setting a property to an array containing an invalid type (#1898) * Emit error when setting a property to an array containing an invalid type * Address PR comments * Address PR comments * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Chayim <chayim@users.noreply.github.com>

…1898) * Emit error when setting a property to an array containing an invalid type * Address PR comments * Address PR comments * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

* Error when setting a property to an array containing an invalid type (#1898) * Emit error when setting a property to an array containing an invalid type * Address PR comments * Address PR comments * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Aliases in WITH...ORDER BY must be valid references (#1897) * Ensure aliases in the ORDER BY segment of a WITH clause are valid references * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * increase code coverage (#1919) * clean unused files * delete v5 decoder * test decoders * fix build * delete unused functions * add deleted nodes and edges to rdb * fix unused function * address review * Storing test artifacts for CI runs (#1906) * Storing test artifacts * PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * sync matrices on parent process before fork (#1931) * sync matrices on parent process before fork * formatting * fix tests * fix bgsave * Error when setting a property to an array containing an invalid type (#1898) * Emit error when setting a property to an array containing an invalid type * Address PR comments * Address PR comments * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * Aliases in WITH...ORDER BY must be valid references (#1897) * Ensure aliases in the ORDER BY segment of a WITH clause are valid references * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * increase code coverage (#1919) * clean unused files * delete v5 decoder * test decoders * fix build * delete unused functions * add deleted nodes and edges to rdb * fix unused function * address review * Storing test artifacts for CI runs (#1906) * Storing test artifacts * PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> * sync matrices on parent process before fork (#1931) * sync matrices on parent process before fork * formatting * fix tests * fix bgsave * fix timeout test Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Chayim <chayim@users.noreply.github.com>

…edisGraph#1898) * Emit error when setting a property to an array containing an invalid type * Address PR comments * Address PR comments * Address PR comments * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>

Emit error when setting a property to an array containing an invalid …

ceed4b5

…type

jeffreylovitz added the bug label Aug 26, 2021

jeffreylovitz requested a review from swilly22 August 26, 2021 19:15

jeffreylovitz self-assigned this Aug 26, 2021

swilly22 suggested changes Aug 28, 2021

View reviewed changes

Address PR comments

42bd377

jeffreylovitz force-pushed the validate-property-array-contents branch from 48545db to 42bd377 Compare August 30, 2021 20:24

swilly22 suggested changes Aug 31, 2021

View reviewed changes

Address PR comments

9ab99e9

jeffreylovitz force-pushed the validate-property-array-contents branch from 2d24cd4 to 9ab99e9 Compare August 31, 2021 21:06

Merge branch 'master' into validate-property-array-contents

747904f

swilly22 suggested changes Sep 8, 2021

View reviewed changes

Address PR comments

11a0381

swilly22 suggested changes Sep 8, 2021

View reviewed changes

Address PR comments

e9505e7

swilly22 approved these changes Sep 9, 2021

View reviewed changes

swilly22 added 2 commits September 9, 2021 09:45

Merge branch 'master' into validate-property-array-contents

0924f0d

Merge branch 'master' into validate-property-array-contents

801a279

swilly22 merged commit 1f66e01 into master Sep 13, 2021

swilly22 deleted the validate-property-array-contents branch September 13, 2021 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Error when setting a property to an array containing an invalid type #1898

Error when setting a property to an array containing an invalid type #1898

Uh oh!

jeffreylovitz commented Aug 26, 2021

Uh oh!

codecov bot commented Aug 26, 2021 •

edited

Loading

Uh oh!

swilly22 Aug 28, 2021

Uh oh!

jeffreylovitz Aug 31, 2021

Uh oh!

swilly22 Aug 28, 2021

Uh oh!

swilly22 Aug 31, 2021

Uh oh!

swilly22 Sep 8, 2021

Uh oh!

jeffreylovitz Sep 8, 2021

Uh oh!

swilly22 Sep 8, 2021

Uh oh!

swilly22 Sep 8, 2021

Uh oh!

jeffreylovitz Sep 8, 2021

Uh oh!

swilly22 Sep 8, 2021

Uh oh!

swilly22 Sep 8, 2021

Uh oh!

swilly22 Sep 8, 2021

Uh oh!

Uh oh!

Error when setting a property to an array containing an invalid type #1898

Error when setting a property to an array containing an invalid type #1898

Uh oh!

Conversation

jeffreylovitz commented Aug 26, 2021

Uh oh!

codecov bot commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Aug 26, 2021 •

edited

Loading