Horton

Thread

Commits

  1. Fix thinkos in LookupFuncName() for function name lookups

  1. Prevent invalid memory access in LookupFuncName

    Alexander Law <exclusion@gmail.com> — 2019-06-24T20:10:54Z

    Hello hackers,

When running on REL_11_STABLE the following query:
CREATE PROCEDURE test_ambiguous_procname(int) as $$ begin end; $$
language plpgsql;
CREATE PROCEDURE test_ambiguous_procname(text) as $$ begin end; $$
language plpgsql;
DROP PROCEDURE test_ambiguous_procname;
under valgrind I get the memory access errors:

2019-06-24 22:21:39.925 MSK|law|regression|5d1122c2.2921|LOG: 
statement: DROP PROCEDURE test_ambiguous_procname;
==00:00:00:07.756 10529== Conditional jump or move depends on
uninitialised value(s)
==00:00:00:07.756 10529==    at 0x4C35E60: __memcmp_sse4_1 (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==00:00:00:07.756 10529==    by 0x2E9A7B: LookupFuncName (parse_func.c:2078)
==00:00:00:07.756 10529==    by 0x2E9D96: LookupFuncWithArgs
(parse_func.c:2141)
==00:00:00:07.756 10529==    by 0x2A2F7E: get_object_address
(objectaddress.c:893)
==00:00:00:07.756 10529==    by 0x31C9C0: RemoveObjects (dropcmds.c:71)
==00:00:00:07.756 10529==    by 0x50D4FF: ExecDropStmt (utility.c:1738)
==00:00:00:07.756 10529==    by 0x5100BC: ProcessUtilitySlow
(utility.c:1580)
==00:00:00:07.756 10529==    by 0x50EDFE: standard_ProcessUtility
(utility.c:835)
==00:00:00:07.756 10529==    by 0x50F07D: ProcessUtility (utility.c:360)
==00:00:00:07.756 10529==    by 0x50B4D2: PortalRunUtility (pquery.c:1178)
==00:00:00:07.756 10529==    by 0x50C169: PortalRunMulti (pquery.c:1324)
==00:00:00:07.756 10529==    by 0x50CEFF: PortalRun (pquery.c:799)
==00:00:00:07.756 10529==  Uninitialised value was created by a stack
allocation
==00:00:00:07.756 10529==    at 0x2E9C31: LookupFuncWithArgs
(parse_func.c:2106)
==00:00:00:07.756 10529==
...
==00:00:00:07.756 10529== Conditional jump or move depends on
uninitialised value(s)
==00:00:00:07.756 10529==    at 0x2E9A7E: LookupFuncName (parse_func.c:2078)
==00:00:00:07.756 10529==    by 0x2E9D96: LookupFuncWithArgs
(parse_func.c:2141)
==00:00:00:07.757 10529==    by 0x2A2F7E: get_object_address
(objectaddress.c:893)
==00:00:00:07.757 10529==    by 0x31C9C0: RemoveObjects (dropcmds.c:71)
==00:00:00:07.757 10529==    by 0x50D4FF: ExecDropStmt (utility.c:1738)
==00:00:00:07.757 10529==    by 0x5100BC: ProcessUtilitySlow
(utility.c:1580)
==00:00:00:07.757 10529==    by 0x50EDFE: standard_ProcessUtility
(utility.c:835)
==00:00:00:07.757 10529==    by 0x50F07D: ProcessUtility (utility.c:360)
==00:00:00:07.757 10529==    by 0x50B4D2: PortalRunUtility (pquery.c:1178)
==00:00:00:07.757 10529==    by 0x50C169: PortalRunMulti (pquery.c:1324)
==00:00:00:07.757 10529==    by 0x50CEFF: PortalRun (pquery.c:799)
==00:00:00:07.757 10529==    by 0x5090FF: exec_simple_query
(postgres.c:1145)
==00:00:00:07.757 10529==  Uninitialised value was created by a stack
allocation
==00:00:00:07.757 10529==    at 0x2E9C31: LookupFuncWithArgs
(parse_func.c:2106)

As I see, the code in LookupFuncName can fall through the "if (nargs ==
-1)" condition and execute memcmp with nargs==-1.
The proposed patch is attached.

Best regards,
Alexander

  2. Re: Prevent invalid memory access in LookupFuncName

    Michael Paquier <michael@paquier.xyz> — 2019-06-25T02:19:56Z

    On Mon, Jun 24, 2019 at 11:10:54PM +0300, Alexander Lakhin wrote:
> When running on REL_11_STABLE the following query:
> CREATE PROCEDURE test_ambiguous_procname(int) as $$ begin end; $$
> language plpgsql;
> CREATE PROCEDURE test_ambiguous_procname(text) as $$ begin end; $$
> language plpgsql;
> DROP PROCEDURE test_ambiguous_procname;
> under valgrind I get the memory access errors:

Thanks!  I have been able to reproduce the problem, and the error is
obvious looking at the code.  I have changed the patch to be more
consistent with HEAD though, returning InvalidOid in the code paths
generating the error.  The logic is the same, but that looked cleaner
to me, and I have added some comments on the way, similarly to what
bfb456c1 has done for HEAD (where LookupFuncNameInternal is doing the
right thing already).  This has been incorrect since aefeb68, so
back-patched down to 10.
--
Michael