Horton

Thread

Commits

  1. Fix erroneous Valgrind markings in AllocSetRealloc.

  1. Possible false valgrind error reports

    Karina Litskevich <litskevichkarina@gmail.com> — 2023-02-14T14:49:27Z

    Hi hackers,

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.

Also, it took me a while to understand what's going on there, so in 0002
patch
I tried to improve comments and renamed a variable. Its name "oldsize"
confused
me. I first thought "oldsize" and "size" represent the same parameters of
the
old and new chunk. But actually "size" is new "chunk->requested_size" and
"oldsize" is old "chksize". So I believe it's better to rename "oldsize"
into
"oldchksize".

Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/

  2. Re: Possible false valgrind error reports

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-14T20:21:45Z

    Karina Litskevich <litskevichkarina@gmail.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

  3. Re: Possible false valgrind error reports

    Karina Litskevich <litskevichkarina@gmail.com> — 2023-02-17T15:58:45Z

    Thank you, I moved comment changes to 0001 and rewrote the fix through Min().

> 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 than chksize >= oldsize and so we can mark this
memory without worries. Region from size to chksize will be marked NOACCESS
later anyway:

/* Ensure any padding bytes are marked NOACCESS. */
VALGRIND_MAKE_MEM_NOACCESS((char *) pointer + size, chksize - size);

I agree that it's not obvious, so I changed the first hunk like this:

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

Any ideas on how to make this place easier to understand and comment above it
concise and clear are welcome.

There is another thing about this version. New line
+ Min(size, oldsize) - chunk->requested_size);
is longer than 80 symbols and I don't know what's the best way to avoid this
without making it look weird.

I also noticed that if RANDOMIZE_ALLOCATED_MEMORY is defined then
randomize_mem()
have already marked this memory UNDEFINED. So we only "may need to adjust
trailing bytes" if RANDOMIZE_ALLOCATED_MEMORY isn't defined. I reflected it in
v2 of 0001 too.

  4. Re: Possible false valgrind error reports

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-21T23:50:06Z

    Karina Litskevich <litskevichkarina@gmail.com> writes:
> Thank you, I moved comment changes to 0001 and rewrote the fix through Min().

Looks good.  I pushed it after a little more fiddling with the comments.
Thanks for the report and patch!

			regards, tom lane