Violation of safety checks on children in `cypher_ast_query` constructor

[cleishm]

Violation of safety checks on children in `cypher_ast_query` constructor

[cleishm]

So the check is there for good reason, as you probably have guessed. I'll shortly add an additional set of helpers, which will allow for cloning/rewriting an AST.

However, to do that, I have to know that any dependencies of a node are also referenced in the node's children. Hence the added checks.

In your case, the problem with the call cypher_ast_query(NULL, 0, (cypher_astnode_t *const *)clauses, n, NULL, 0, range);, is the fact that you're passing NULL for the children argument.

Simple fix - just use cypher_ast_query(NULL, 0, (cypher_astnode_t *const *)clauses, n, (cypher_astnode_t *const *)clauses, n, range);.

Also, you can get rid of the cast away from const, if you change clauses to be const cypher_astnode_t *. The casting to const, i.e. (cypher_astnode_t *const *), while quite annoying, can't be avoided - so don't beat yourself up about it. It's a known issue in C: http://c-faq.com/ansi/constmismatch.html

Here's your example re-written:

        const cypher_astnode_t *clauses[n];
	for(uint i = 0; i < n; i ++) {
		clauses[i] = cypher_ast_query_get_clause(master_ast->root, i + start_offset);
	}
	struct cypher_input_range range = {};
	ast->root = cypher_ast_query(NULL, 0, (cypher_astnode_t *const *)clauses, n, (cypher_astnode_t *const *)clauses, n, range);

Cheers,
Chris

[jeffreylovitz]

Thanks for the rewrite! That does look a lot more reasonable haha.

I have already switched the children argument as you recommend here, but because that array in the constructed AST node is heap-allocated separately, it causes a memory leak:

 24 bytes in 3 blocks are definitely lost in loss record 16 of 222
    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    by 0x8B2FFC6: mdup (util.h:147)
    by 0x8B31065: cypher_astnode_init (ast.c:802)
    by 0x8B42018: cypher_ast_query (ast_query.c:60)

clauses are allocated in the same chunk as the AST node itself, so we can just free the constructed node, but I don't think there is an exposed free routine that will handle the children?

[cleishm]

Ah, yes, that's an additional problem.

To clean up AST nodes, you should use cypher_ast_free(astnode) - not free(...) directly. However, that will recursively free the children - so you're going to end up in trouble because those children you just added are already part of another tree.

So forget what I just wrote - that's not going to work safely.

What you need is to clone the clauses before adding them to a new tree. Fortunately, I just pushed such a method to master. Warning though - I was in the middle of writing some tests, and have pushed without them. So whilst I think it's all correct, I haven't finished verifying that.

So, adding that little step, your code would look something like:

        cypher_astnode_t *clauses[n];
	for(uint i = 0; i < n; i ++) {
		clauses[i] = cypher_ast_clone(cypher_ast_query_get_clause(master_ast->root, i + start_offset));
                if (clauses[i] == NULL) {
                        // TODO: cleanup and handle error
                        return -1;
                }
	}
	struct cypher_input_range range = {};
	ast->root = cypher_ast_query(NULL, 0, (cypher_astnode_t *const *)clauses, n, (cypher_astnode_t *const *)clauses, n, range);
        ...
        ...
        ...
        cypher_ast_free(ast->root);

See if that works?

[cleishm]

On consideration, despite the danger, there is some merit to allowing the approach you described.

So I've adjusted the way the recursive release of children is handled, and added a cypher_astnode_free() method to the exposed API. This will allow you to release a node without releasing its children. As long as you take responsibility for releasing the children (or use children that will get released as part of releasing a whole other tree), you should be ok. Dangerous, but ok if you're careful.

[jeffreylovitz]

@cleishm That is perfect, thank you! These scoped ASTs improve our abstractions greatly, but since only one is really "active" at any time for us, deep copies would be a bit wasteful.

Your cypher_astnode_free() solution looks ideal for our purposes; I'll try that out tomorrow.

Thanks for the quick resolution!