Re: BUG #16801: Invalid memory access on WITH RECURSIVE with nested WITHs

On Tue, Feb 23, 2021 at 09:00:00AM +0300, Alexander Lakhin wrote:
> I've found out that the list implementation doesn't support the
> following usage pattern:
> List *testList = NIL;
> ListCell *testCell;
>
> testList = lcons(NIL, testList);
> testCell = list_head(testList);
> ...
> testList = lcons(NIL, testList);
>
> elog(INFO, "lfirst(testCell): %p", lfirst(testCell)); // prints
> 0x7f7f7f7f7f7f7f7f when compiled with -DUSE_VALGRIND
>
> (Such list manipulation is happening in that makeDependencyGraphWalker
> call.)

Thanks for the reminder, I was not able to get around this issue.
Anyway.. What's happening here is that the second lcons() call does
new_head_cell() as testList is not NIL. This itself calls
enlarge_list(), followed by wipe_mem() which invalidates the position
of the list head previously stored. Oops.

Coming back to the original problem, as you say there is some
confusion about the list operation that we had better clarify. From
what I can see cstate->innerwiths stores a List of Lists, and in each
passage the code tries to build a new list appended to
cstate->innerwiths. Using a ListCell to be able to track where the
new list head is located is awkward on HEAD (the business with cell1),
and there should be no need to keep around the reference to the first
element innerwiths, as long as you save the result in a new, separate,
List.

The second case in checkWellFormedRecursionWalker() is equally
dangerous. I have been playing with subselects and more recursions
and could not trigger a problem with -DUSE_VALGRIND, but let's be
safe.

So attached is a patch to take care of this, with a regression test
based on what has been sent upthread. This solves the issue for me.

Thoughts?
--
Michael