Re: [HACKERS] Multiple false-positive warnings from Valgrind

2017-04-05 Thread Noah Misch 
On Wed, Mar 29, 2017 at 12:34:52PM +0900, Michael Paquier wrote:
> On Thu, Mar 23, 2017 at 5:15 PM, Michael Paquier
>  wrote:
> > On Tue, Mar 21, 2017 at 10:57 PM, Aleksander Alekseev
> >  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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multiple false-positive warnings from Valgrind

2017-04-01 Thread Michael Paquier 
On Sat, Apr 1, 2017 at 2:51 PM, Noah Misch  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multiple false-positive warnings from Valgrind

2017-03-31 Thread Noah Misch 
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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multiple false-positive warnings from Valgrind

2017-03-31 Thread Aleksander Alekseev 
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


signature.asc
Description: PGP signature

Re: [HACKERS] Multiple false-positive warnings from Valgrind

2017-03-28 Thread Kyotaro HORIGUCHI 
At Wed, 29 Mar 2017 12:34:52 +0900, Michael Paquier  
wrote in 
> On Thu, Mar 23, 2017 at 5:15 PM, Michael Paquier
>  wrote:
> > On Tue, Mar 21, 2017 at 10:57 PM, Aleksander Alekseev
> >  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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multiple false-positive warnings from Valgrind

2017-03-28 Thread Michael Paquier 
On Thu, Mar 23, 2017 at 5:15 PM, Michael Paquier
 wrote:
> On Tue, Mar 21, 2017 at 10:57 PM, Aleksander Alekseev
>  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Multiple false-positive warnings from Valgrind

2017-03-23 Thread Michael Paquier 
On Tue, Mar 21, 2017 at 10:57 PM, Aleksander Alekseev
 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Multiple false-positive warnings from Valgrind

2017-03-21 Thread Aleksander Alekseev 
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]:

```
0x00a160b4 in pg_b64_encode (src=0xffefffb10 [...] at base64.c:80
80  *p++ = _base64[(buf >> 12) & 0x3f];
(gdb) monitor get_vbits 0xffefffb10 10
  

0x008a7c36 in hex_encode (
src=0xffefffbc0 [...] at encode.c:132
132 *dst++ = hextbl[(*src >> 4) & 0xF];
(gdb) monitor get_vbits 0xffefffbc0 32
       
```

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


signature.asc
Description: PGP signature