Hi Tatsuo, Jian,
I would like to disagree with one part of this, late and after it has
been agreed to. Sorry for coming back to it only now.
>> I also intend to add below at the beginning of nfa_state_free
>> ``````
>> /* state at the free-list head was freed by the previous call */
>> if (unlikely(winstate->nfaStateFree == state))
>> elog(ERROR, "double free of RPR NFA state");
>> ``````
>
> Looks good to me.
I do not think this guard catches what its name claims. It detects
one thing only: the same pointer freed twice in a row, with no other
free in between. The comment says so itself -- it compares against
the state the previous call pushed. One unrelated free between the
two and it passes:
nfa_state_free(A) head = A
nfa_state_free(B) head = B, B->next = A
nfa_state_free(A) head is B, not A -- the check passes
A->next = B, head = A
The free list is now A -> B -> A and nothing reported it. Two
consecutive frees of one pointer is also the shape a reader is most
likely to catch in review, so the guard covers the case we would find
anyway and lets through the case that survives review.
> Ok. You are not confident the errors never happen. If so, using
> elog(ERROR) makes sense.
There were paths that failed to free, so the doubt is not an abstract
one. That was in February and March, and they were fixed. A great
deal has changed since, and I agree the memory handling is worth
verifying again -- I would rather that verification be done under
Valgrind than by a check compiled into release builds.
> On the hand if we are confident that the error is truley unlikely
> happen, using elog(ERROR) is just a waste of CPU.
Agreed, and it is worth saying where this one would land.
nfa_state_free() is called from eight sites, five of them inside the
per-row loops of nfa_match() and the advance paths, so whatever we put
in it is paid per state, per row, per partition.
And a double free is in fact already detected here, by a check that
does not have the limitation above.
In an ordinary build nothing is freed at all: nfa_state_free() pushes
the pointer onto a private list and the chunk stays live in
partcontext, so there is nothing for anything to observe. That is why
the function already carries:
#ifdef USE_VALGRIND
/* real free so Valgrind catches use-after-free instead of
recycling */
pfree(state);
#else
state->next = winstate->nfaStateFree;
winstate->nfaStateFree = state;
#endif
Under USE_VALGRIND that branch makes every free a real pfree(), and
since nothing ever assigns nfaStateFree there it stays NULL, so every
nfa_state_make() allocates afresh from MemoryContextAlloc(). Every
state therefore travels the ordinary palloc/pfree path.
USE_VALGRIND also defines MEMORY_CONTEXT_CHECKING on its own
(pg_config_manual.h), which turns on AllocSetFree()'s own guard:
/* Test for previously-freed chunk */
if (unlikely(chunk->requested_size == InvalidAllocSize))
elog(ERROR, "detected double pfree in %s %p",
set->header.name, chunk);
That is the check being proposed, already in the tree and without the
consecutive-free limitation: it tests a mark on the chunk itself
rather than the position of a list head, so it fires on any second
free. The comment above it names the same hazard -- "the result would
be a corrupted freelist that allows this chunk to get re-allocated
twice." Valgrind then adds the other half, since the freed chunk is
made NOACCESS: a state recycled while a caller still held it is
reported as an invalid read or write.
To be definite about it: this class of problem has to be verified by
other means than the one proposed -- in a Valgrind run, where the
detection already exists and is general, rather than by a branch
compiled into release builds.
Best regards,
Henson
