Horton

Thread

Commits

  1. Code review for psql's helpSQL() function.

  1. BUG #16837: Invalid memory access on \h in psql

    The Post Office <noreply@postgresql.org> — 2021-01-26T07:00:00Z

    The following bug has been logged on the website:

Bug reference:      16837
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 13.1
Operating system:   Ubuntu 20.04
Description:        

When executing in psql (under valgrind):
\h\

valgrind detects the following error:
==00:00:00:00.000 3226182== 
==00:00:00:04.045 3226182== Conditional jump or move depends on
uninitialised value(s)
==00:00:00:04.045 3226182==    at 0x1396CB: helpSQL (help.c:600)
==00:00:00:04.045 3226182==    by 0x120705: exec_command_help
(command.c:1507)
==00:00:00:04.045 3226182==    by 0x1252CD: exec_command (command.c:351)
==00:00:00:04.045 3226182==    by 0x1258A3: HandleSlashCmds
(command.c:222)
==00:00:00:04.045 3226182==    by 0x13B166: MainLoop (mainloop.c:502)
==00:00:00:04.045 3226182==    by 0x1238B3: process_file (command.c:3921)
==00:00:00:04.045 3226182==    by 0x14357A: main (startup.c:400)
==00:00:00:04.045 3226182==  Uninitialised value was created by a heap
allocation
==00:00:00:04.045 3226182==    at 0x483B7F3: malloc (in
/usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==00:00:00:04.045 3226182==    by 0x4AB2A1C: initPQExpBuffer
(pqexpbuffer.c:94)
==00:00:00:04.045 3226182==    by 0x13D9CE: psql_scan_slash_option
(psqlscanslash.l:563)
==00:00:00:04.045 3226182==    by 0x1206B6: exec_command_help
(command.c:1493)
==00:00:00:04.045 3226182==    by 0x1252CD: exec_command (command.c:351)
==00:00:00:04.045 3226182==    by 0x1258A3: HandleSlashCmds
(command.c:222)
==00:00:00:04.045 3226182==    by 0x13B166: MainLoop (mainloop.c:502)
==00:00:00:04.045 3226182==    by 0x1238B3: process_file (command.c:3921)
==00:00:00:04.045 3226182==    by 0x14357A: main (startup.c:400)
==00:00:00:04.045 3226182== 
{
   <insert_a_suppression_name_here>
   Memcheck:Cond
   fun:helpSQL
   fun:exec_command_help
   fun:exec_command
   fun:HandleSlashCmds
   fun:MainLoop
   fun:process_file
   fun:main
}
No help available for "\".
Try \h with no arguments to see available help.

psql is started with the following command line:
valgrind --leak-check=no --track-origins=yes --time-stamp=yes
--read-var-info=yes  \
 --gen-suppressions=all --suppressions=src/tools/valgrind.supp \
 --trace-children=yes $PGROOT/usr/local/pgsql/bin/psql

  2. Re: BUG #16837: Invalid memory access on \h in psql

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-01-26T11:17:09Z

    At Tue, 26 Jan 2021 07:00:00 +0000, PG Bug reporting form <noreply@postgresql.org> wrote in 
> When executing in psql (under valgrind):
> \h\
> 
> valgrind detects the following error:
> ==00:00:00:00.000 3226182== 
> ==00:00:00:04.045 3226182== Conditional jump or move depends on
> uninitialised value(s)
> ==00:00:00:04.045 3226182==    at 0x1396CB: helpSQL (help.c:600)
> ==00:00:00:04.045 3226182==    by 0x120705: exec_command_help
> (command.c:1507)
> ==00:00:00:04.045 3226182==    by 0x1252CD: exec_command (command.c:351)
> ==00:00:00:04.045 3226182==    by 0x1258A3: HandleSlashCmds
> (command.c:222)

This is reproducible on master HEAD. helpSQL assumes that the first
word is longer than two characters and the second word exists. It also
doesn't care overruns. Addition to those issues, it miscounts the
length of the first two words if the third word exists.

=# \h ALTER VIEX HOGE
<prints help only of "ALTER VIEW"!, not of "ALTER *">

 >	if (x > 1)			/* Nothing on first pass - try the opening
 >						 * word(s) */
 >	{
 >		wordlen = j = 1;
!>		while (topic[j] != ' ' && j++ < len)
 >			wordlen++;
 >		if (x == 2)
 >		{
 >			j++;
!>			while (topic[j] != ' ' && j++ <= len)
 >				wordlen++;
 >		}

So we should check j before accessing topic[j] and count the length
correctly. The attached fixes that. This seems to be very old code.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  3. Re: BUG #16837: Invalid memory access on \h in psql

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-01-26T16:11:22Z

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Tue, 26 Jan 2021 07:00:00 +0000, PG Bug reporting form <noreply@postgresql.org> wrote in 
>> When executing in psql (under valgrind):
>> \h\
>> valgrind detects the following error:
>> ==00:00:00:00.000 3226182== 
>> ==00:00:00:04.045 3226182== Conditional jump or move depends on
>> uninitialised value(s)

> This is reproducible on master HEAD. helpSQL assumes that the first
> word is longer than two characters and the second word exists. It also
> doesn't care overruns. Addition to those issues, it miscounts the
> length of the first two words if the third word exists.

Weirdly, valgrind isn't whining about this for me.  But I agree that
that loop is unsafe.  There are other problems too I think: neither
the initialization of "output" nor the calculation of nl_count seem
to be done sanely.  This function really needs thoroughgoing review :-(

			regards, tom lane

  4. Re: BUG #16837: Invalid memory access on \h in psql

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-01-27T01:34:35Z

    At Tue, 26 Jan 2021 11:11:22 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > At Tue, 26 Jan 2021 07:00:00 +0000, PG Bug reporting form <noreply@postgresql.org> wrote in 
> >> When executing in psql (under valgrind):
> >> \h\
> >> valgrind detects the following error:
> >> ==00:00:00:00.000 3226182== 
> >> ==00:00:00:04.045 3226182== Conditional jump or move depends on
> >> uninitialised value(s)
> 
> > This is reproducible on master HEAD. helpSQL assumes that the first
> > word is longer than two characters and the second word exists. It also
> > doesn't care overruns. Addition to those issues, it miscounts the
> > length of the first two words if the third word exists.
> 
> Weirdly, valgrind isn't whining about this for me.  But I agree that
> that loop is unsafe.  There are other problems too I think: neither
> the initialization of "output" nor the calculation of nl_count seem
> to be done sanely.  This function really needs thoroughgoing review :-(

It looks far better now. Thanks!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center