Thread: [HACKERS] Multiple false-positive warnings from Valgrind
Hello. I need a little help. Recently I've decided to run PostgreSQL under Valgrind according to wiki description [1]. Lots of warnings are generated [2] but it is my understanding that all of them are false-positive. For instance I've found these two reports particularly interesting: ``` ==00:00:40:40.161 7677== Use of uninitialised value of size 8 ==00:00:40:40.161 7677== at 0xA15FF5: pg_b64_encode (base64.c:68) ==00:00:40:40.161 7677== by 0x6FFE85: scram_build_verifier (auth-scram.c:348) ==00:00:40:40.161 7677== by 0x6F3F76: encrypt_password (crypt.c:171) ==00:00:40:40.161 7677== by 0x68F40C: CreateRole (user.c:403) ==00:00:40:40.161 7677== by 0x85D53A: standard_ProcessUtility (utility.c:716) ==00:00:40:40.161 7677== by 0x85CCC7: ProcessUtility (utility.c:353) ==00:00:40:40.161 7677== by 0x85BD22: PortalRunUtility (pquery.c:1165) ==00:00:40:40.161 7677== by 0x85BF20: PortalRunMulti (pquery.c:1308) ==00:00:40:40.161 7677== by 0x85B4A0: PortalRun (pquery.c:788) ==00:00:40:40.161 7677== by 0x855672: exec_simple_query (postgres.c:1101) ==00:00:40:40.161 7677== by 0x8597BB: PostgresMain (postgres.c:4066) ==00:00:40:40.161 7677== by 0x7C6322: BackendRun (postmaster.c:4317) ==00:00:40:40.161 7677== Uninitialised value was created by a stack allocation ==00:00:40:40.161 7677== at 0x6FFDB7: scram_build_verifier (auth-scram.c:328) ==00:00:40:40.593 7677== Use of uninitialised value of size 8 ==00:00:40:40.593 7677== at 0x8A7C36: hex_encode (encode.c:132) ==00:00:40:40.593 7677== by 0x6FFEF5: scram_build_verifier (auth-scram.c:355) ==00:00:40:40.593 7677== by 0x6F3F76: encrypt_password (crypt.c:171) ==00:00:40:40.593 7677== by 0x68F40C: CreateRole (user.c:403) ==00:00:40:40.593 7677== by 0x85D53A: standard_ProcessUtility (utility.c:716) ==00:00:40:40.593 7677== by 0x85CCC7: ProcessUtility (utility.c:353) ==00:00:40:40.593 7677== by 0x85BD22: PortalRunUtility (pquery.c:1165) ==00:00:40:40.593 7677== by 0x85BF20: PortalRunMulti (pquery.c:1308) ==00:00:40:40.593 7677== by 0x85B4A0: PortalRun (pquery.c:788) ==00:00:40:40.593 7677== by 0x855672: exec_simple_query (postgres.c:1101) ==00:00:40:40.593 7677== by 0x8597BB: PostgresMain (postgres.c:4066) ==00:00:40:40.593 7677== by 0x7C6322: BackendRun (postmaster.c:4317) ==00:00:40:40.593 7677== Uninitialised value was created by a stack allocation ==00:00:40:40.593 7677== at 0x6FFDB7: scram_build_verifier (auth-scram.c:328) ==00:00:40:40.593 7677== ``` And here is what I see in GDB [3]: ``` 0x0000000000a160b4 in pg_b64_encode (src=0xffefffb10 [...] at base64.c:80 80 *p++ = _base64[(buf >> 12) & 0x3f]; (gdb) monitor get_vbits 0xffefffb10 10 ffffffff ffffffff ffff 0x00000000008a7c36 in hex_encode ( src=0xffefffbc0 [...] at encode.c:132 132 *dst++ = hextbl[(*src >> 4) & 0xF]; (gdb) monitor get_vbits 0xffefffbc0 32 ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ffffffff ``` So Valgrind thinks that in both cases first argument is completely uninitialized which is very doubtful to say the least :) There are also lots of memory leak reports which could be found in [2]. I got a strong feeling that maybe I'm doing something wrong. Here are exact script I'm using to build [4], install and run PostgreSQL under Valgrind [5]. Naturally USE_VALGRIND in declared in pg_config_manual.h. Valgrind version is 3.12 and an environment in general is Arch Linux. Could you please give a little piece of advice? Or maybe a wiki page is just a bit outdated? [1] https://wiki.postgresql.org/wiki/Valgrind [2] http://afiskon.ru/s/8a/390698e914_valgrind.tgz [3] http://afiskon.ru/s/09/c4f6231679_pgvg.txt [4] https://github.com/afiskon/pgscripts/blob/master/quick-build.sh [5] https://github.com/afiskon/pgscripts/blob/master/valgrind.sh -- Best regards, Aleksander Alekseev
On Tue, Mar 21, 2017 at 10:57 PM, Aleksander Alekseev <a.alekseev@postgrespro.ru> wrote: > Recently I've decided to run PostgreSQL under Valgrind according to wiki > description [1]. Lots of warnings are generated [2] but it is my > understanding that all of them are false-positive. For instance I've > found these two reports particularly interesting: > > ``` > ==00:00:40:40.161 7677== Use of uninitialised value of size 8 > ==00:00:40:40.161 7677== at 0xA15FF5: pg_b64_encode (base64.c:68) > ==00:00:40:40.161 7677== by 0x6FFE85: scram_build_verifier (auth-scram.c:348) > ==00:00:40:40.161 7677== by 0x6F3F76: encrypt_password (crypt.c:171) > ==00:00:40:40.161 7677== by 0x68F40C: CreateRole (user.c:403) > ==00:00:40:40.161 7677== by 0x85D53A: standard_ProcessUtility (utility.c:716) > ==00:00:40:40.161 7677== by 0x85CCC7: ProcessUtility (utility.c:353) > ==00:00:40:40.161 7677== by 0x85BD22: PortalRunUtility (pquery.c:1165) > ==00:00:40:40.161 7677== by 0x85BF20: PortalRunMulti (pquery.c:1308) > ==00:00:40:40.161 7677== by 0x85B4A0: PortalRun (pquery.c:788) > ==00:00:40:40.161 7677== by 0x855672: exec_simple_query (postgres.c:1101) > ==00:00:40:40.161 7677== by 0x8597BB: PostgresMain (postgres.c:4066) > ==00:00:40:40.161 7677== by 0x7C6322: BackendRun (postmaster.c:4317) > ==00:00:40:40.161 7677== Uninitialised value was created by a stack allocation > ==00:00:40:40.161 7677== at 0x6FFDB7: scram_build_verifier (auth-scram.c:328) I can see those warnings as well after calling a code path of scram_build_verifier(), and I have a hard time seeing that as nothing else than a false positive as you do. All those warnings go away if you just initialize just do MemSet(salt, 0, SCRAM_SALT_LEN) before calling pg_backend_random() but this data is filled in with RAND_bytes() afterwards (if built with openssl). The estimated lengths of the encoding are also correct. I don't see immediately what's wrong here, this deserves a second look... -- Michael
On Thu, Mar 23, 2017 at 5:15 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Mar 21, 2017 at 10:57 PM, Aleksander Alekseev > <a.alekseev@postgrespro.ru> wrote: >> Recently I've decided to run PostgreSQL under Valgrind according to wiki >> description [1]. Lots of warnings are generated [2] but it is my >> understanding that all of them are false-positive. For instance I've >> found these two reports particularly interesting: >> >> ``` >> ==00:00:40:40.161 7677== Use of uninitialised value of size 8 >> ==00:00:40:40.161 7677== at 0xA15FF5: pg_b64_encode (base64.c:68) >> ==00:00:40:40.161 7677== by 0x6FFE85: scram_build_verifier (auth-scram.c:348) >> ==00:00:40:40.161 7677== by 0x6F3F76: encrypt_password (crypt.c:171) >> ==00:00:40:40.161 7677== by 0x68F40C: CreateRole (user.c:403) >> ==00:00:40:40.161 7677== by 0x85D53A: standard_ProcessUtility (utility.c:716) >> ==00:00:40:40.161 7677== by 0x85CCC7: ProcessUtility (utility.c:353) >> ==00:00:40:40.161 7677== by 0x85BD22: PortalRunUtility (pquery.c:1165) >> ==00:00:40:40.161 7677== by 0x85BF20: PortalRunMulti (pquery.c:1308) >> ==00:00:40:40.161 7677== by 0x85B4A0: PortalRun (pquery.c:788) >> ==00:00:40:40.161 7677== by 0x855672: exec_simple_query (postgres.c:1101) >> ==00:00:40:40.161 7677== by 0x8597BB: PostgresMain (postgres.c:4066) >> ==00:00:40:40.161 7677== by 0x7C6322: BackendRun (postmaster.c:4317) >> ==00:00:40:40.161 7677== Uninitialised value was created by a stack allocation >> ==00:00:40:40.161 7677== at 0x6FFDB7: scram_build_verifier (auth-scram.c:328) > > I can see those warnings as well after calling a code path of > scram_build_verifier(), and I have a hard time seeing that as nothing > else than a false positive as you do. All those warnings go away if > you just initialize just do MemSet(salt, 0, SCRAM_SALT_LEN) before > calling pg_backend_random() but this data is filled in with > RAND_bytes() afterwards (if built with openssl). The estimated lengths > of the encoding are also correct. I don't see immediately what's wrong > here, this deserves a second look... And it seems to me that this is caused by the routines of OpenSSL. When building without --with-openssl, using the fallback implementations of SHA256 and RAND_bytes I see no warnings generated by scram_build_verifier... I think it makes most sense to discard that from the list of open items. -- Michael
At Wed, 29 Mar 2017 12:34:52 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in <CAB7nPqQKdxZ-C67OzU+FiHhtU7NOb8qazjrb-9j0u8P0qzCNMA@mail.gmail.com> > On Thu, Mar 23, 2017 at 5:15 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: > > On Tue, Mar 21, 2017 at 10:57 PM, Aleksander Alekseev > > <a.alekseev@postgrespro.ru> wrote: > >> Recently I've decided to run PostgreSQL under Valgrind according to wiki > >> description [1]. Lots of warnings are generated [2] but it is my > >> understanding that all of them are false-positive. For instance I've > >> found these two reports particularly interesting: > >> > >> ``` > >> ==00:00:40:40.161 7677== Use of uninitialised value of size 8 > >> ==00:00:40:40.161 7677== at 0xA15FF5: pg_b64_encode (base64.c:68) > >> ==00:00:40:40.161 7677== by 0x6FFE85: scram_build_verifier (auth-scram.c:348) ... > > I can see those warnings as well after calling a code path of > > scram_build_verifier(), and I have a hard time seeing that as nothing > > else than a false positive as you do. All those warnings go away if > > you just initialize just do MemSet(salt, 0, SCRAM_SALT_LEN) before > > calling pg_backend_random() but this data is filled in with > > RAND_bytes() afterwards (if built with openssl). The estimated lengths > > of the encoding are also correct. I don't see immediately what's wrong > > here, this deserves a second look... > > And it seems to me that this is caused by the routines of OpenSSL. > When building without --with-openssl, using the fallback > implementations of SHA256 and RAND_bytes I see no warnings generated > by scram_build_verifier... I think it makes most sense to discard that > from the list of open items. FWIW a document of the function says that, https://www.openssl.org/docs/man1.0.1/crypto/RAND_bytes.html > The contents of buf is mixed into the entropy pool before > retrieving the new pseudo-random bytes unless disabled at compile > time (see FAQ). This isn't saying that RAND_bytes does the same thing but something similar can be happening there. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
Hi Kyotaro, > > And it seems to me that this is caused by the routines of OpenSSL. > > When building without --with-openssl, using the fallback > > implementations of SHA256 and RAND_bytes I see no warnings generated > > by scram_build_verifier... I think it makes most sense to discard that > > from the list of open items. > > FWIW a document of the function says that, > > https://www.openssl.org/docs/man1.0.1/crypto/RAND_bytes.html > > > The contents of buf is mixed into the entropy pool before > > retrieving the new pseudo-random bytes unless disabled at compile > > time (see FAQ). > > This isn't saying that RAND_bytes does the same thing but > something similar can be happening there. OK, turned out that warnings regarding uninitialized values disappear after removing --with-openssl. That's a good thing. What about all these memory leak reports [1]? If I see them should I just ignore them or, if reports look false positive, suggest a patch that modifies a Valgrind suppression file? In other words what is current consensus in community regarding Valgrind and it's reports? [1] http://afiskon.ru/s/47/871f1e21ef_valgrind.txt.gz -- Best regards, Aleksander Alekseev
On Fri, Mar 31, 2017 at 04:40:07PM +0300, Aleksander Alekseev wrote: > > > And it seems to me that this is caused by the routines of OpenSSL. > > > When building without --with-openssl, using the fallback > > > implementations of SHA256 and RAND_bytes I see no warnings generated > > > by scram_build_verifier... I think it makes most sense to discard that > > > from the list of open items. > > > > FWIW a document of the function says that, > > > > https://www.openssl.org/docs/man1.0.1/crypto/RAND_bytes.html > > > > > The contents of buf is mixed into the entropy pool before > > > retrieving the new pseudo-random bytes unless disabled at compile > > > time (see FAQ). > > > > This isn't saying that RAND_bytes does the same thing but > > something similar can be happening there. > > OK, turned out that warnings regarding uninitialized values disappear > after removing --with-openssl. That's a good thing. Does this remove the noise under --with-openssl? --- a/src/port/pg_strong_random.c +++ b/src/port/pg_strong_random.c @@ -104,7 +104,10 @@ pg_strong_random(void *buf, size_t len) */#if defined(USE_OPENSSL_RANDOM) if (RAND_bytes(buf,len) == 1) + { + VALGRIND_MAKE_MEM_DEFINED(buf, len); return true; + } return false; /* > What about all these memory leak reports [1]? If I see them should I just > ignore them or, if reports look false positive, suggest a patch that > modifies a Valgrind suppression file? In other words what is current > consensus in community regarding Valgrind and it's reports? Pass --leak-check=no; PostgreSQL intentionally leaks a lot.
On Sat, Apr 1, 2017 at 2:51 PM, Noah Misch <noah@leadboat.com> wrote:
> Does this remove the noise under --with-openssl?
>
> --- a/src/port/pg_strong_random.c
> +++ b/src/port/pg_strong_random.c
> @@ -104,7 +104,10 @@ pg_strong_random(void *buf, size_t len)
> */
> #if defined(USE_OPENSSL_RANDOM)
> if (RAND_bytes(buf, len) == 1)
> + {
> + VALGRIND_MAKE_MEM_DEFINED(buf, len);
> return true;
> + }
> return false;
>
> /*
Actually, no. I'll dig more into the options of valgrind to see if I
am missing something here..
--
Michael
On Wed, Mar 29, 2017 at 12:34:52PM +0900, Michael Paquier wrote: > On Thu, Mar 23, 2017 at 5:15 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: > > On Tue, Mar 21, 2017 at 10:57 PM, Aleksander Alekseev > > <a.alekseev@postgrespro.ru> wrote: > >> Recently I've decided to run PostgreSQL under Valgrind according to wiki > >> description [1]. Lots of warnings are generated [2] but it is my > >> understanding that all of them are false-positive. For instance I've > >> found these two reports particularly interesting: > >> > >> ``` > >> ==00:00:40:40.161 7677== Use of uninitialised value of size 8 > >> ==00:00:40:40.161 7677== at 0xA15FF5: pg_b64_encode (base64.c:68) > >> ==00:00:40:40.161 7677== by 0x6FFE85: scram_build_verifier (auth-scram.c:348) > >> ==00:00:40:40.161 7677== by 0x6F3F76: encrypt_password (crypt.c:171) > >> ==00:00:40:40.161 7677== by 0x68F40C: CreateRole (user.c:403) > >> ==00:00:40:40.161 7677== by 0x85D53A: standard_ProcessUtility (utility.c:716) > >> ==00:00:40:40.161 7677== by 0x85CCC7: ProcessUtility (utility.c:353) > >> ==00:00:40:40.161 7677== by 0x85BD22: PortalRunUtility (pquery.c:1165) > >> ==00:00:40:40.161 7677== by 0x85BF20: PortalRunMulti (pquery.c:1308) > >> ==00:00:40:40.161 7677== by 0x85B4A0: PortalRun (pquery.c:788) > >> ==00:00:40:40.161 7677== by 0x855672: exec_simple_query (postgres.c:1101) > >> ==00:00:40:40.161 7677== by 0x8597BB: PostgresMain (postgres.c:4066) > >> ==00:00:40:40.161 7677== by 0x7C6322: BackendRun (postmaster.c:4317) > >> ==00:00:40:40.161 7677== Uninitialised value was created by a stack allocation > >> ==00:00:40:40.161 7677== at 0x6FFDB7: scram_build_verifier (auth-scram.c:328) > > > > I can see those warnings as well after calling a code path of > > scram_build_verifier(), and I have a hard time seeing that as nothing > > else than a false positive as you do. All those warnings go away if > > you just initialize just do MemSet(salt, 0, SCRAM_SALT_LEN) before > > calling pg_backend_random() but this data is filled in with > > RAND_bytes() afterwards (if built with openssl). The estimated lengths > > of the encoding are also correct. I don't see immediately what's wrong > > here, this deserves a second look... > > And it seems to me that this is caused by the routines of OpenSSL. > When building without --with-openssl, using the fallback > implementations of SHA256 and RAND_bytes I see no warnings generated > by scram_build_verifier... I think it makes most sense to discard that > from the list of open items. This defect has roughly the gravity of a compiler warning. Dropped from open items on that basis.