Out-of-bounds write and incorrect detection of trigger file in pg_standby

On Wed, Jan 14, 2015 at 8:24 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:

In pg_standby.c, we use a 32-byte buffer in CheckForExternalTrigger to
which is read the content of the trigger file defined by -f:
CheckForExternalTrigger(void)
{
char buf[32];
[...]
if ((len = read(fd, buf, sizeof(buf))) < 0)
{
stuff();
}

if (len < sizeof(buf))
buf[len] = '\0';
However it happens that if the file read contains 32 bytes or more, we
enforce \0 in a position out of bounds. While looking at that, I also
noticed that pg_standby can trigger a failover as long as the
beginning of buf matches either "smart" or "fast", but I think we
should check for a perfect match instead of a comparison of the first
bytes, no?

Attached is a patch to fix the out-of-bound issue as well as
improvements for the detection of the trigger file content. I think
that the out-of-bound fix should be backpatched, while the
improvements for the trigger file are master-only. Coverity has
pointed out the out-of-bound issue.

I would imagine that the goal might have been to accept not only smart
but also smart\r and smart\n and smart\r\n. It's a little weird that
we also accept smartypants, though.

Instead of doing this:

if (len < sizeof(buf))
buf[len] = '\0';

...I would suggest making the size of the buffer one greater than the
size of the read(), and then always nul-terminating the buffer. It
seems to me that would make the code easier to reason about.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On Thu, Jan 15, 2015 at 7:13 AM, Robert Haas <robertmhaas@gmail.com> wrote:

Instead of doing this:

if (len < sizeof(buf))
buf[len] = '\0';

...I would suggest making the size of the buffer one greater than the
size of the read(), and then always nul-terminating the buffer. It
seems to me that would make the code easier to reason about.

How about the attached then? This way we still detect the same way any
invalid values:
-       if ((len = read(fd, buf, sizeof(buf))) < 0)
+       if ((len = read(fd, buf, sizeof(buf) - 1)) < 0)
Regards,
-- 
Michael

On Wed, Jan 14, 2015 at 9:27 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Thu, Jan 15, 2015 at 7:13 AM, Robert Haas <robertmhaas@gmail.com> wrote:

Instead of doing this:

if (len < sizeof(buf))
buf[len] = '\0';

...I would suggest making the size of the buffer one greater than the
size of the read(), and then always nul-terminating the buffer. It
seems to me that would make the code easier to reason about.

How about the attached then? This way we still detect the same way any
invalid values:
-       if ((len = read(fd, buf, sizeof(buf))) < 0)
+       if ((len = read(fd, buf, sizeof(buf) - 1)) < 0)

Committed and back-patched all the way.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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