Обсуждение: Out-of-bounds write and incorrect detection of trigger file in pg_standby

Hi all,

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.
Regards,
--
Michael

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

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