Date: Tue, 20 Feb 2018 18:08:38 +0300
From: Aleksander Alekseev <a.alekseev@postgrespro.ru>
To: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: [PATCH] Add a few suppression rules for Valgrind
Message-ID: <20180220150838.GD18315@e733.localdomain>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="ILuaRSyQpoVaJ1HG"
Content-Disposition: inline
User-Agent: Mutt/1.9.3 (2018-01-21)
Precedence: bulk


--ILuaRSyQpoVaJ1HG
Content-Type: multipart/mixed; boundary="5gxpn/Q6ypwruk0T"
Content-Disposition: inline


--5gxpn/Q6ypwruk0T
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hello hackers,

I decided to run the code from master branch under Valgrind and
discovered that it reports some errors.

There are multiple reports like this one (seems to be a false alarm):

```
Invalid read of size 16
   at 0x605F488: __wcsnlen_sse4_1 (in /usr/lib/libc-2.26.so)
   by 0x604F5C2: wcsrtombs (in /usr/lib/libc-2.26.so)
   by 0x5FE4C41: wcstombs (in /usr/lib/libc-2.26.so)
   by 0x6FFAFC: wchar2char (pg_locale.c:1641)
   by 0x6937A0: str_tolower (formatting.c:1599)
   by 0x6F88E8: lower (oracle_compat.c:50)
   by 0x43097D: ExecInterpExpr (execExprInterp.c:678)
   by 0x48B201: ExecEvalExpr (executor.h:286)
   by 0x48BBD6: tfuncInitialize (nodeTableFuncscan.c:369)
   by 0x48B91E: tfuncFetchRows (nodeTableFuncscan.c:299)
   by 0x48B245: TableFuncNext (nodeTableFuncscan.c:65)
   by 0x4462E6: ExecScanFetch (execScan.c:95)
Address 0x513adab0 is 7,808 bytes inside a recently re-allocated
  block of size 16,384 alloc'd
   at 0x4C2CEDF: malloc (vg_replace_malloc.c:299)
   by 0x7E2807: AllocSetAlloc (aset.c:934)
   by 0x7EF481: palloc (mcxt.c:848)
   by 0x42D2A7: ExprEvalPushStep (execExpr.c:2131)
   by 0x42D7DB: ExecPushExprSlots (execExpr.c:2286)
   by 0x42D723: ExecInitExprSlots (execExpr.c:2265)
   by 0x429B77: ExecBuildProjectionInfo (execExpr.c:370)
   by 0x44A39B: ExecAssignProjectionInfo (execUtils.c:457)
   by 0x4733FE: ExecInitNestLoop (nodeNestloop.c:308)
   by 0x4442F0: ExecInitNode (execProcnode.c:290)
   by 0x43C5EB: InitPlan (execMain.c:1043)
   by 0x43B31C: standard_ExecutorStart (execMain.c:262)
```

Also there are many reports like this one (definitely false alarm):

```
Syscall param epoll_pwait(sigmask) points to unaddressable byte(s)
    at 0x60A2F90: epoll_pwait (in /usr/lib/libc-2.26.so)
    by 0x5F7B24: WaitEventSetWaitBlock (latch.c:1048)
    by 0x5F79E8: WaitEventSetWait (latch.c:1000)
    by 0x493A79: secure_read (be-secure.c:170)
    by 0x4A3DE2: pq_recvbuf (pqcomm.c:963)
    by 0x4A3EAA: pq_getbyte (pqcomm.c:1006)
    by 0x627AD0: SocketBackend (postgres.c:339)
    by 0x628032: ReadCommand (postgres.c:512)
    by 0x62D243: PostgresMain (postgres.c:4086)
    by 0x581240: BackendRun (postmaster.c:4412)
    by 0x5808BE: BackendStartup (postmaster.c:4084)
    by 0x57CA20: ServerLoop (postmaster.c:1757)
Address 0x0 is not stack'd, malloc'd or (recently) free'd
```

I suggest a patch that adds corresponding suppression rules to the
src/tools/valgrind.supp file. Though I'm having difficulties
understanding why Valgrind complains on wcstombs and thus I can't
explain why in fact everything is OK (or it's actually not?). Hopefully
someone in the mailing list can explain this behavior. For now I left a
TODO mark in the comment.

TWIMC the full report can be found here:

https://afiskon.ru/s/3c/736207c9d2_valgrind-2018-02-20.tgz

--=20
Best regards,
Aleksander Alekseev

--5gxpn/Q6ypwruk0T
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="valgrind.patch"

diff --git a/src/tools/valgrind.supp b/src/tools/valgrind.supp
index af03051260..accf989047 100644
--- a/src/tools/valgrind.supp
+++ b/src/tools/valgrind.supp
@@ -212,3 +212,47 @@
    Memcheck:Cond
    fun:PyObject_Realloc
 }
+
+
+# Accroding to the man page for epoll_pwait:
+# """
+# The sigmask argument may be specified as NULL, in which case epoll_pwait() is
+# equivalent to epoll_wait().
+# """
+# Valgrind seems to be unaware of this and complains about passing NULL as a
+# sigmask, so we need this suppression rule.
+{
+   epoll_pwait_null_sigmask
+   Memcheck:Param
+   epoll_pwait(sigmask)
+   fun:epoll_pwait
+   fun:WaitEventSetWaitBlock
+   fun:WaitEventSetWait
+
+   ...
+   fun:ServerLoop
+}
+
+
+# TODO: why this is harmless?
+{
+   wcstombs
+   Memcheck:Addr16
+   fun:__wcsnlen_sse4_1
+   fun:wcsrtombs
+   fun:wcstombs
+
+   ...
+   fun:ExecInterpExpr
+}
+
+{
+   <insert_a_suppression_name_here>
+   Memcheck:Cond
+   fun:__wcsnlen_sse4_1
+   fun:wcsrtombs
+   fun:wcstombs
+
+   ...
+   fun:ExecInterpExpr
+}

--5gxpn/Q6ypwruk0T--

--ILuaRSyQpoVaJ1HG
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEECAokn0QZCoIHW4GpEEnCBLXCDtYFAlqMOfMACgkQEEnCBLXC
DtaaUBAA7BfI8hhAb03UfVWlBbE7Pr3TwM8bfHSlurObLt0DzQwZMrUwF7g5xmaE
A6T/Mkd//etCjw3a7e0Rf0rrqy6DRXufdojPvpuD/HSlSdbQ7WBeB8M+sTlnP2uF
EBfX/8TVaS+AzTpuW/77PJOACHuXb2q54ldAl/Q60GZXhGo8QCoI23HDL6eVDF86
qDoB0HJ8mHyv25HZH6GqEFwHPzpOfwqY/zspUQwbqdarRu+dyClkhvr+LSFo2e47
tXCgdMDPk3kviI3bi5ON9HCeaGfxelxqwyb+OkbbgRNZGLVLb5THsVcbH8/OLKbZ
FBlR01zZ+tgGwLygJQH/HmjKEdG6y9H94y3gyDi2sK2eZZM3J5MwRvvxmcCONxpT
ARfWEF0Ee0re9LRReLdiQzZJTvj11ZUDeJhhOFdSER7jPPyNhEiFq91MfTRguks7
rwl1OKyECbt7yc4e+atjkFCqRTMqPm6GrkZebUugwiDFfxuWbbKD7K7vhToeAOiH
Jh+SGpiS5Ty1/iH3YOvDSGI7sGGOzaDGBYadIjriichb94H/St5Jd1o8pwwlvzL/
abKC253y2VyK5I98FN8jpZKknuhsmLt5zUsEx825flKZiogMNZbgkCCenYxUIfdr
ChG+uqA52hA5lUToUERqyIhiuLO5OgfRqU7pbvrlZh91lvpsgX0=
=RGSd
-----END PGP SIGNATURE-----

--ILuaRSyQpoVaJ1HG--