Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)

On 03/22/2016 03:27 PM, Aleksander Alekseev wrote:
> diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
> index 1ff5728..a10c078 100644
> --- a/src/backend/commands/tablespace.c
> +++ b/src/backend/commands/tablespace.c
> @@ -669,6 +669,11 @@ destroy_tablespace_directories(Oid tablespaceoid, bool redo)
> char *subfile;
> struct stat st;
>
> + /*
> + * Prevents MemorySanitizer's "use-of-uninitialized-value" warning
> + */
> + memset(&st, 0, sizeof(st));
> +
> linkloc_with_version_dir = psprintf("pg_tblspc/%u/%s", tablespaceoid,
> TABLESPACE_VERSION_DIRECTORY);

Why does MemorySanitizer complain about that? Calling stat(2) is
supposed to fill in all the fields we look at, right?

> diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
> index 924bebb..498e7bd 100644
> --- a/src/backend/utils/cache/inval.c
> +++ b/src/backend/utils/cache/inval.c
> @@ -330,6 +330,7 @@ AddCatcacheInvalidationMessage(InvalidationListHeader *hdr,
> SharedInvalidationMessage msg;
>
> Assert(id < CHAR_MAX);
> + memset(&msg, 0, sizeof(msg));
> msg.cc.id = (int8) id;
> msg.cc.dbId = dbId;
> msg.cc.hashValue = hashValue;

Right after this, we have:

> /*
> * Define padding bytes in SharedInvalidationMessage structs to be
> * defined. Otherwise the sinvaladt.c ringbuffer, which is accessed by
> * multiple processes, will cause spurious valgrind warnings about
> * undefined memory being used. That's because valgrind remembers the
> * undefined bytes from the last local process's store, not realizing that
> * another process has written since, filling the previously uninitialized
> * bytes
> */
> VALGRIND_MAKE_MEM_DEFINED(&msg, sizeof(msg));

Do we need both?

> diff --git a/src/backend/utils/mmgr/aset.c b/src/backend/utils/mmgr/aset.c
> index d26991e..46ab8a2 100644
> --- a/src/backend/utils/mmgr/aset.c
> +++ b/src/backend/utils/mmgr/aset.c
> @@ -850,7 +850,7 @@ AllocSetAlloc(MemoryContext context, Size size)
> blksize <<= 1;
>
> /* Try to allocate it */
> - block = (AllocBlock) malloc(blksize);
> + block = (AllocBlock) calloc(1, blksize);
>
> /*
> * We could be asking for pretty big blocks here, so cope if malloc
> @@ -861,7 +861,7 @@ AllocSetAlloc(MemoryContext context, Size size)
> blksize >>= 1;
> if (blksize < required_size)
> break;
> - block = (AllocBlock) malloc(blksize);
> + block = (AllocBlock) calloc(1, blksize);
> }
>
> if (block == NULL)

I think this goes too far. You're zeroing all palloc'd memory, even if
it's going to be passed to palloc0(), and zeroed there. It might even
silence legitimate warnings, if there's code somewhere that does
palloc(), and accesses some of it before initializing. Plus it's a
performance hit.

> diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
> index e7826a4..4bbd4d2 100644
> --- a/src/test/regress/regress.c
> +++ b/src/test/regress/regress.c
> @@ -1022,6 +1022,11 @@ test_atomic_uint64(void)
> uint64 expected;
> int i;
>
> + /*
> + * Prevents MemorySanitizer's "use-of-uninitialized-value" warning
> + */
> + memset(&var, 0, sizeof(var));
> +
> pg_atomic_init_u64(&var, 0);
>
> if (pg_atomic_read_u64(&var) != 0)

What's going on here? Surely pg_atomic_init_u64() should initialize the
value?

- Heikki