Обсуждение: Thinko in processing of SHM message size info?
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
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
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
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