Re: Possible false valgrind error reports
Karina Litskevich <litskevichkarina@gmail.com>
From: Karina Litskevich <litskevichkarina@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>,
Noah Misch <noah@leadboat.com>
Date: 2023-02-17T15:58:45Z
Lists: pgsql-hackers
Attachments
- v2-0002-Change-variable-name-in-AllocSetRealloc.patch (text/x-patch) patch v2-0002
- v2-0001-Fix-VALGRIND_MAKE_MEM_DEFINED-calls.patch (text/x-patch) patch v2-0001
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.
Commits
-
Fix erroneous Valgrind markings in AllocSetRealloc.
- b3e184a5d4fd 16.0 landed
- dc44180f6e17 14.8 landed
- 99e74cd235a5 13.11 landed
- 463bef38332e 12.15 landed
- 21bd818d05fb 11.20 landed
- f6a55c1d5593 15.3 landed