Re: Possible false valgrind error reports

Karina Litskevich <litskevichkarina(at)gmail(dot)com> writes:
> In 82d0a46ea32 AllocSetRealloc() was changed to allow decreasing size of
> external chunks and give memory back to the malloc pool. Two
> VALGRIND_MAKE_MEM_UNDEFINED() calls were not changed to work properly in the
> case of decreasing size: they can mark memory behind the new allocated
> memory
> UNDEFINED. If this memory was already allocated and initialized, it's
> expected
> to be DEFINED. So it can cause false valgrind error reports. I fixed it in
> 0001 patch.

Hmm, I see the concern: adjusting the Valgrind marking of bytes beyond the
newly-realloced block is wrong because it might tromp on memory allocated
in another way. However, I'm not sure about the details of your patch.

The first hunk in 0001 doesn't seem quite right yet:

* old allocation.
*/
#ifdef USE_VALGRIND
- if (oldsize > chunk->requested_size)
+ if (size > chunk->requested_size && oldsize > chunk->requested_size)
VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + chunk->requested_size,
oldsize - chunk->requested_size);
#endif

If size < oldsize, aren't we still doing the wrong thing? Seems like
maybe it has to be like

if (size > chunk->requested_size && oldsize > chunk->requested_size)
VALGRIND_MAKE_MEM_UNDEFINED((char *) pointer + chunk->requested_size,
Min(size, oldsize) - chunk->requested_size);

* allocation; it could have been as small as one byte. We have to be
* conservative and just mark the entire old portion DEFINED.
*/
- VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
+ if (size >= oldsize)
+ VALGRIND_MAKE_MEM_DEFINED(pointer, oldsize);
+ else
+ VALGRIND_MAKE_MEM_DEFINED(pointer, size);
#endif

This is OK, though I wonder if it'd read better as

+ VALGRIND_MAKE_MEM_DEFINED(pointer, Min(size, oldsize));

I've not thought hard about whether I like the variable renaming proposed
in 0002. I do suggest though that those comment changes are an integral
part of the bug fix and hence belong in 0001.

regards, tom lane