Thread: Thinko in processing of SHM message size info?

Thinko in processing of SHM message size info?

From
Antonin Houska
Date:
Can anyone please explain why the following patch shouldn't be applied?

diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
index 126cb07..4cd52ac 100644
--- a/src/backend/storage/ipc/shm_mq.c
+++ b/src/backend/storage/ipc/shm_mq.c
@@ -584,7 +584,7 @@ shm_mq_receive(shm_mq_handle *mqh, Size *nbytesp, void **datap, bool nowait)            if
(mqh->mqh_partial_bytes+ rb > sizeof(Size))                lengthbytes = sizeof(Size) - mqh->mqh_partial_bytes;
  else 
-                lengthbytes = rb - mqh->mqh_partial_bytes;
+                lengthbytes = rb;            memcpy(&mqh->mqh_buffer[mqh->mqh_partial_bytes], rawdata,
 lengthbytes);            mqh->mqh_partial_bytes += lengthbytes; 


I'm failing to understand why anything should be subtracted. Note that the
previous iteration must have called shm_mq_inc_bytes_read(), so "rb" should
not include anything of mqh->mqh_partial_bytes. Thanks.

--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at



Re: Thinko in processing of SHM message size info?

From
Robert Haas
Date:
On Thu, Aug 6, 2015 at 9:04 AM, Antonin Houska <ah@cybertec.at> wrote:
> Can anyone please explain why the following patch shouldn't be applied?
>
> diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
> index 126cb07..4cd52ac 100644
> --- a/src/backend/storage/ipc/shm_mq.c
> +++ b/src/backend/storage/ipc/shm_mq.c
> @@ -584,7 +584,7 @@ shm_mq_receive(shm_mq_handle *mqh, Size *nbytesp, void **datap, bool nowait)
>                         if (mqh->mqh_partial_bytes + rb > sizeof(Size))
>                                 lengthbytes = sizeof(Size) - mqh->mqh_partial_bytes;
>                         else
> -                               lengthbytes = rb - mqh->mqh_partial_bytes;
> +                               lengthbytes = rb;
>                         memcpy(&mqh->mqh_buffer[mqh->mqh_partial_bytes], rawdata,
>                                    lengthbytes);
>                         mqh->mqh_partial_bytes += lengthbytes;
>
>
> I'm failing to understand why anything should be subtracted. Note that the
> previous iteration must have called shm_mq_inc_bytes_read(), so "rb" should
> not include anything of mqh->mqh_partial_bytes. Thanks.

Hmm, I think you are correct.  This would matter in the case where the
message length word was read in more than two chunks.  I don't *think*
that's possible right now because I believe the only systems where
MAXIMUM_ALIGNOF < sizeof(Size) are those with MAXIMUM_ALIGNOF == 4 and
sizeof(Size) == 8.  However, if we had systems where MAXIMUM_ALIGNOF
== 4 and sizeof(Size) == 16, or systems where MAXIMUM_ALIGNOF == 2 and
sizeof(Size) == 8, this would be a live bug.

Thanks for reviewing; I'll go push this change.

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



Re: Thinko in processing of SHM message size info?

From
Robert Haas
Date:
On Thu, Aug 6, 2015 at 1:24 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Aug 6, 2015 at 9:04 AM, Antonin Houska <ah@cybertec.at> wrote:
>> Can anyone please explain why the following patch shouldn't be applied?
>>
>> diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
>> index 126cb07..4cd52ac 100644
>> --- a/src/backend/storage/ipc/shm_mq.c
>> +++ b/src/backend/storage/ipc/shm_mq.c
>> @@ -584,7 +584,7 @@ shm_mq_receive(shm_mq_handle *mqh, Size *nbytesp, void **datap, bool nowait)
>>                         if (mqh->mqh_partial_bytes + rb > sizeof(Size))
>>                                 lengthbytes = sizeof(Size) - mqh->mqh_partial_bytes;
>>                         else
>> -                               lengthbytes = rb - mqh->mqh_partial_bytes;
>> +                               lengthbytes = rb;
>>                         memcpy(&mqh->mqh_buffer[mqh->mqh_partial_bytes], rawdata,
>>                                    lengthbytes);
>>                         mqh->mqh_partial_bytes += lengthbytes;
>>
>>
>> I'm failing to understand why anything should be subtracted. Note that the
>> previous iteration must have called shm_mq_inc_bytes_read(), so "rb" should
>> not include anything of mqh->mqh_partial_bytes. Thanks.
>
> Hmm, I think you are correct.  This would matter in the case where the
> message length word was read in more than two chunks.  I don't *think*
> that's possible right now because I believe the only systems where
> MAXIMUM_ALIGNOF < sizeof(Size) are those with MAXIMUM_ALIGNOF == 4 and
> sizeof(Size) == 8.  However, if we had systems where MAXIMUM_ALIGNOF
> == 4 and sizeof(Size) == 16, or systems where MAXIMUM_ALIGNOF == 2 and
> sizeof(Size) == 8, this would be a live bug.

Hmm, actually, maybe it is a live bug anyway, because the if statement
tests > rather than >=.  If we've read 4 bytes and exactly 4 more
bytes are available, we'd set lengthbytes to 0 instead of 4.  Oops.

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



Re: Thinko in processing of SHM message size info?

From
Antonin Houska
Date:
Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Aug 6, 2015 at 1:24 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Thu, Aug 6, 2015 at 9:04 AM, Antonin Houska <ah@cybertec.at> wrote:
> >> Can anyone please explain why the following patch shouldn't be applied?
> >>
> >> diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
> >> index 126cb07..4cd52ac 100644
> >> --- a/src/backend/storage/ipc/shm_mq.c
> >> +++ b/src/backend/storage/ipc/shm_mq.c
> >> @@ -584,7 +584,7 @@ shm_mq_receive(shm_mq_handle *mqh, Size *nbytesp, void **datap, bool nowait)
> >>                         if (mqh->mqh_partial_bytes + rb > sizeof(Size))
> >>                                 lengthbytes = sizeof(Size) - mqh->mqh_partial_bytes;
> >>                         else
> >> -                               lengthbytes = rb - mqh->mqh_partial_bytes;
> >> +                               lengthbytes = rb;
> >>                         memcpy(&mqh->mqh_buffer[mqh->mqh_partial_bytes], rawdata,
> >>                                    lengthbytes);
> >>                         mqh->mqh_partial_bytes += lengthbytes;
> >>
> >>
> >> I'm failing to understand why anything should be subtracted. Note that the
> >> previous iteration must have called shm_mq_inc_bytes_read(), so "rb" should
> >> not include anything of mqh->mqh_partial_bytes. Thanks.
> >

> > Hmm, I think you are correct.  This would matter in the case where the
> > message length word was read in more than two chunks.  I don't *think*
> > that's possible right now because I believe the only systems where
> > MAXIMUM_ALIGNOF < sizeof(Size) are those with MAXIMUM_ALIGNOF == 4 and
> > sizeof(Size) == 8. However, if we had systems where MAXIMUM_ALIGNOF
> > == 4 and sizeof(Size) == 16, or systems where MAXIMUM_ALIGNOF == 2 and
> > sizeof(Size) == 8, this would be a live bug.

I ought to admit that I didn't think about the specific combinations of
MAXIMUM_ALIGNOF and sizeof(Size), and considered the problem rather rare
(maybe also because it can't happen on my workstation). But the next your
consideration makes sense to me:

> Hmm, actually, maybe it is a live bug anyway, because the if statement
> tests > rather than >=.  If we've read 4 bytes and exactly 4 more
> bytes are available, we'd set lengthbytes to 0 instead of 4.  Oops.

--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at