Re: Possible false valgrind error reports

From: Karina Litskevich <litskevichkarina(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Noah Misch <noah(at)leadboat(dot)com>
Subject: Re: Possible false valgrind error reports
Date: 2023-02-17 15:58:45
Message-ID: CACiT8ia=83qScbh4qifi8y6gxygDoTsyppuCY2JhjZ4hnQhp_w@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

Attachment Content-Type Size
v2-0002-Change-variable-name-in-AllocSetRealloc.patch text/x-patch 4.7 KB
v2-0001-Fix-VALGRIND_MAKE_MEM_DEFINED-calls.patch text/x-patch 2.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Jonathan S. Katz 2023-02-17 16:19:44 Re: The output sql generated by pg_dump for a create function refers to a modified table name
Previous Message Dmitry Dolgov 2023-02-17 15:46:43 Re: pg_stat_statements and "IN" conditions