Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow traversal patterns outside of the appropriate clauses #1431

Merged
merged 3 commits into from Nov 18, 2020

Conversation

jeffreylovitz
Copy link
Contributor

@jeffreylovitz jeffreylovitz commented Nov 16, 2020

Cypher allows traversal patterns to appear in a number of unexpected places, such as:

MATCH (a) RETURN (a)-[]->()
MATCH (a) RETURN a ORDER BY (a)-[]->()
MATCH (a {prop: ()-[]->()}) RETURN a

These constructions don't seem useful to me, and they often cause crashes because QueryGraph construction collects all paths in an AST:
https://github.com/RedisGraph/RedisGraph/blob/master/src/graph/query_graph.c#L301

This PR disallows all such constructions by introducing a whitelist of the AST nodes that can validly have traversals as their children.

Resolves #1428

@jeffreylovitz jeffreylovitz self-assigned this Nov 16, 2020
@jeffreylovitz jeffreylovitz added this to In progress in RedisGraph 2.4 via automation Nov 16, 2020
Copy link
Collaborator

@swilly22 swilly22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fuzzer starting to show dividends! this is great.

@@ -447,3 +444,32 @@ def test29_invalid_filter_non_boolean_constant(self):
# Non-existent properties are treated as NULLs, which are boolean in Cypher's 3-valued logic.
query = """MATCH (a) WHERE a.fakeprop RETURN a"""
redis_graph.query(query)

# Encountering traversals as property values or ORDER BY expressions should raise compile-time errors.
def test30_unexpected_traversals(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
Consider

q0 = """MATCH (a {prop: ()-[]->()}) RETURN a"""
q1 = """MATCH (a) RETURN a ORDER BY (a)-[]->()"""
q2 = """MATCH (a) RETURN (a)-[]->()"""
queries = [q0, q1, q2]
for q in queries:
    try:            
            redis_graph.query(q)
            assert(False)
        except redis.exceptions.ResponseError as e:
            assert("Encountered path traversal" in e.message)
            pass

@@ -299,6 +299,36 @@ static AST_Validation _ValidateFunctionCalls(const cypher_astnode_t *node,
return res;
}

static AST_Validation _Validate_Path_Locations(const cypher_astnode_t *root) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment giving some information about what this function do

return AST_VALID;
}


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra new line

Comment on lines 308 to 316
if(root_type == CYPHER_AST_PATTERN ||
root_type == CYPHER_AST_MATCH ||
root_type == CYPHER_AST_MERGE ||
root_type == CYPHER_AST_WITH ||
root_type == CYPHER_AST_NAMED_PATH ||
root_type == CYPHER_AST_UNARY_OPERATOR ||
root_type == CYPHER_AST_BINARY_OPERATOR) {
valid_caller = true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can improve a bit by relocating this block:

for(uint i = 0; i < nchildren; i ++) {
    const cypher_astnode_t *child = cypher_astnode_get_child(root, i);
    if(cypher_astnode_type(child) == CYPHER_AST_PATTERN_PATH) {
        const cypher_astnode_type_t root_type = cypher_astnode_type(root);
        if(root_type == CYPHER_AST_PATTERN ||
			root_type == CYPHER_AST_MATCH ||
			root_type == CYPHER_AST_MERGE ||
			root_type == CYPHER_AST_WITH ||
			root_type == CYPHER_AST_NAMED_PATH ||
			root_type == CYPHER_AST_UNARY_OPERATOR ||
			root_type == CYPHER_AST_BINARY_OPERATOR) {
		valid_caller = true;
	}
        if(!valid_caller) {
            QueryCtx_SetError("Encountered path traversal in unsupported location '%s'", cypher_astnode_typestr(cypher_astnode_type(child)));
            return AST_INVALID;
}...

Also might be faster to match invalid_caller if the list of clauses is shorter.

RedisGraph 2.4 automation moved this from In progress to Review in progress Nov 17, 2020
RedisGraph 2.4 automation moved this from Review in progress to Reviewer approved Nov 18, 2020
@swilly22 swilly22 merged commit 0f53447 into master Nov 18, 2020
RedisGraph 2.4 automation moved this from Reviewer approved to Done Nov 18, 2020
@swilly22 swilly22 deleted the restrict-traversal-locations branch November 18, 2020 08:05
jeffreylovitz added a commit that referenced this pull request Nov 30, 2020
* Disallow traversal patterns outside of the appropriate clauses

* Address PR comments

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
(cherry picked from commit 0f53447)
swilly22 added a commit that referenced this pull request Dec 2, 2020
* Fuzzy test crashes 2 (#1429)

* Validate number of RETURNs in UNION query

* Improve error handling and messages with invalid WHERE clauses

* Revert changes to optimize_cartesian_product

* Add run-time error for non-boolean predicate constants in runtime values

(cherry picked from commit ebfc507)

* improve graph loading time by noting multi-edge matrices (#1426)

* improve graph loading time by noting multi-edge matices

* compute multi-edge only once

(cherry picked from commit 514fa25)

* Explicitly assign allocation type of SIValue primitives (#1435)

(cherry picked from commit 5d6fdfc)

* Disallow property assignment to complex data types (#1432)

* Disallow property assignment to complex data types

* Address PR comments

* Capture errors before committing updates

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
(cherry picked from commit 524f171)

* Don't assert on failed MERGE updates (#1437)

* Don't assert on failed MERGE updates

* Update op_merge.c

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
(cherry picked from commit b2dadd8)

* Disallow traversal patterns outside of the appropriate clauses (#1431)

* Disallow traversal patterns outside of the appropriate clauses

* Address PR comments

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
(cherry picked from commit 0f53447)

* add redis-cli parameter example (#1333)

* add redis-cli parameter example

I could not find how to set parameters on `redis-cli` on the documentation, so I added it here. Is there a better place? maybe https://oss.redislabs.com/redisgraph/commands/?

* * add a second parameter

to make it clear how to use more than a single parameter

Co-authored-by: Guy Korland <gkorland@gmail.com>
(cherry picked from commit 3f19bd1)

* Fix warnings reported by fb-infer (#1440)

* Fix warnings reported by fb-infer

* validate algebraic expression structure on evaluation

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
Co-authored-by: swilly22 <roi@redislabs.com>
(cherry picked from commit eefb8ef)

* Delete redundant check to create edge schemas (#1444)

(cherry picked from commit 17663a6)

* Fixed bug in OSX build instructions (#1448)

CPP is the environment variable for the preprocessor executable, CXX specifies the compiler path.

(cherry picked from commit 88048ee)

* Disallow MERGE of entities with null properties (#1442)

* Disallow MERGE of entities with null properties

* Fix memory leak

* Move error-handling functions to errors.c

* Fix NULL serialization logic

* Address PR comments

* Address PR comments

* Move all error-handling logic to errors.c

* Fix memory leak

* Fix unit test

* Initialize ErrorCtx thread-local key

* reuse TLS error context, set and raise error in one call

* remove unused include

* Reset error breakpoint after every query

* Don't delete property update maps on encountering NULL values

* Add Valgrind suppression

* Expose ErrorCtx getter for identical logic in handler macro

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
Co-authored-by: swilly22 <roi@redislabs.com>
(cherry picked from commit c972f72)

* new php client redislabs-redisgraph-php added (#1461)

* new php client redislabs-redisgraph-php added

* columns are aligned.

* Update clients.md

* Update clients.md

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
(cherry picked from commit bbd5993)

* Add new rust client with async support (#1456)

Co-authored-by: Guy Korland <gkorland@gmail.com>
Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
(cherry picked from commit 081c863)

* validate clauses following a RETURN clause (#1460)

* validate clauses following a RETURN clause

* relocated return clause sequance validation

* Update ast_validations.c

(cherry picked from commit 2157919)

* Add C# clients (#1469)

(cherry picked from commit 682a5ba)

* switch from assert to debug assert (#1463)

* switch from assert to debug assert

* Continue converting to debug assert

* Revert changes to RG.h

* switch from system assert to RedisModule_Assert

* re-order include sequence

Co-authored-by: Jeffrey Lovitz <jeffrey.lovitz@gmail.com>
(cherry picked from commit 8345677)

* Add stars to clients (#1462)

* Add stars to clients

* Update clients.md

(cherry picked from commit b3e4222)

* Add stars to clients (#1462) (#1464)

* Add stars to clients

* Update clients.md

(cherry picked from commit b3e4222)

* Optimize cartesian product scoping (#1446)

* Don't recurse into previous scopes when optimizing Cartesian Products

* Fix crash on migrating cartesian products with constant filters

* Don't assert on non-filter branch head

* Avoid memory leak

* Fix memory leak

(cherry picked from commit 9b82f8b)

* bump version to 2.2.9

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
Co-authored-by: bc² <odanoburu@users.noreply.github.com>
Co-authored-by: Christoph Zimmermann <40485189+chrisAtRedis@users.noreply.github.com>
Co-authored-by: Mehmet Korkmaz <mehmet@mkorkmaz.com>
Co-authored-by: Thomas Profelt <office@protom.eu>
Co-authored-by: Guy Korland <gkorland@gmail.com>
pnxguide pushed a commit to CMU-SPEED/RedisGraph that referenced this pull request Mar 22, 2023
…Graph#1431)

* Disallow traversal patterns outside of the appropriate clauses

* Address PR comments

Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

RedisGraph crashes on nested traversals in ORDER expressions
2 participants