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 |
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 |