Re: 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(dot)paquier(at)gmail(dot)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