BUG #14615: ReplicationOriginShmemInit Memory access cross-border

On Mon, Apr 10, 2017 at 3:26 PM, <bret.shao@outlook.com> wrote:

MemSet(replication_states, 0, ReplicationOriginShmemSize()); in function
ReplicationOriginShmemInit cause cross-border,because that start address of
the share memory allocated is replication_states_ctl, but call MemSet to
initialize this memory start from replication_states which is variable
states's address in struct ReplicationStateCtl.so call MemSet to set 0 with
the total size of this share memory will cross border of this share memory.

Although, this cross-border will not caused the system failure due to share
memory allocation strategy after my analysis. but i still believe we
shouldn't do like this.

Fix suggestion:
change to
MemSet(replication_states_ctl, 0, ReplicationOriginShmemSize()); then move
to the beginning of if statement.

Yes, there is a mistake here, but I don't agree with your solution. It
seems to me that using mul_size(max_replication_slots,
sizeof(ReplicationState)) is the way to go as you would reinitialize
what has been set in tranche_id. Per se the attached.
--
Michael

Hi Michael,
Thanks for your quickly response!
I think maybe you have a little misunderstanding with my solution.

My solution is that
if (!found)
{
int i;
MemSet(replication_states_ctl, 0, ReplicationOriginShmemSize());
replication_states_ctl->tranche_id = LWLockNewTrancheId();
replication_states_ctl->tranche.name = "ReplicationOrigins";
replication_states_ctl->tranche.array_base =
&replication_states[0].lock;
replication_states_ctl->tranche.array_stride =
sizeof(ReplicationState);

//MemSet(replication_states, 0, ReplicationOriginShmemSize());

for (i = 0; i < max_replication_slots; i++)
LWLockInitialize(&replication_states[i].lock,
replication_states_ctl->tranche_id);
}
So I think it’s easier for understanding code.
What do you think?

Thanks.
Bret

发送自 Windows 10 版邮件<https://go.microsoft.com/fwlink/?LinkId=550986&gt;应用

发件人: Michael Paquier<mailto:michael.paquier@gmail.com>
发送时间: 2017年4月10日 14:39
收件人: bret.shao@outlook.com<mailto:bret.shao@outlook.com>
抄送: PostgreSQL mailing lists<mailto:pgsql-bugs@postgresql.org>; Andres Freund<mailto:andres@anarazel.de>
主题: Re: [BUGS] BUG #14615: ReplicationOriginShmemInit Memory access cross-border

On Mon, Apr 10, 2017 at 3:26 PM, <bret.shao@outlook.com> wrote:

MemSet(replication_states, 0, ReplicationOriginShmemSize()); in function
ReplicationOriginShmemInit cause cross-border,because that start address of
the share memory allocated is replication_states_ctl, but call MemSet to
initialize this memory start from replication_states which is variable
states's address in struct ReplicationStateCtl.so call MemSet to set 0 with
the total size of this share memory will cross border of this share memory.

Although, this cross-border will not caused the system failure due to share
memory allocation strategy after my analysis. but i still believe we
shouldn't do like this.

Fix suggestion:
change to
MemSet(replication_states_ctl, 0, ReplicationOriginShmemSize()); then move
to the beginning of if statement.

Yes, there is a mistake here, but I don't agree with your solution. It
seems to me that using mul_size(max_replication_slots,
sizeof(ReplicationState)) is the way to go as you would reinitialize
what has been set in tranche_id. Per se the attached.
--
Michael

On Mon, Apr 10, 2017 at 4:02 PM, shao bret <bret.shao@outlook.com> wrote:

I think maybe you have a little misunderstanding with my solution.

Without a proper patch it is hard to have a clear opinion in this case.

My solution is that
[...]
So I think it’s easier for understanding code.

What do you think?

One way or the other would be fine, at the end the last call will
likely come from Andres in CC as he is the author and committer of
replication origins. To be honest, I find the use of mul_size()
cleaner here, but that's just an opinion.
--
Michael

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

Please help to see the patch file.
Thanks.
Bret

发送自 Windows 10 版邮件<https://go.microsoft.com/fwlink/?LinkId=550986&gt;应用

发件人: Michael Paquier<mailto:michael.paquier@gmail.com>
发送时间: 2017年4月11日 11:05
收件人: shao bret<mailto:bret.shao@outlook.com>
抄送: PostgreSQL mailing lists<mailto:pgsql-bugs@postgresql.org>; Andres Freund<mailto:andres@anarazel.de>
主题: Re: 答复: [BUGS] BUG #14615: ReplicationOriginShmemInit Memory access cross-border

On Mon, Apr 10, 2017 at 4:02 PM, shao bret <bret.shao@outlook.com> wrote:

I think maybe you have a little misunderstanding with my solution.

Without a proper patch it is hard to have a clear opinion in this case.

My solution is that
[...]
So I think it’s easier for understanding code.

What do you think?

One way or the other would be fine, at the end the last call will
likely come from Andres in CC as he is the author and committer of
replication origins. To be honest, I find the use of mul_size()
cleaner here, but that's just an opinion.
--
Michael

On 2017-04-10 15:38:56 +0900, Michael Paquier wrote:

diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index 5eaf863e02..0aa468789c 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -473,7 +473,8 @@ ReplicationOriginShmemInit(void)

replication_states_ctl->tranche_id = LWTRANCHE_REPLICATION_ORIGIN;

-		MemSet(replication_states, 0, ReplicationOriginShmemSize());
+		MemSet(replication_states, 0,
+			   mul_size(max_replication_slots, sizeof(ReplicationState)));

What's the benefit of using mul_size here? That's usually only
beneficial in the original size computation - during use/initialization
an actual error should be impossible.

To me the right fix seems to be to just do:
-		replication_states_ctl->tranche_id = LWTRANCHE_REPLICATION_ORIGIN;
-
-		MemSet(replication_states, 0, ReplicationOriginShmemSize());
+		MemSet(replication_states_ctl, 0, ReplicationOriginShmemSize());
+
+		replication_states_ctl->tranche_id = LWTRANCHE_REPLICATION_ORIGIN;

No?

Greetings,

Andres Freund

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

On 2017-04-10 07:02:06 +0000, shao bret wrote:

Hi Michael,
Thanks for your quickly response!
I think maybe you have a little misunderstanding with my solution.

My solution is that
if (!found)
{
int i;
MemSet(replication_states_ctl, 0, ReplicationOriginShmemSize());
replication_states_ctl->tranche_id = LWLockNewTrancheId();
replication_states_ctl->tranche.name = "ReplicationOrigins";
replication_states_ctl->tranche.array_base =
&replication_states[0].lock;
replication_states_ctl->tranche.array_stride =
sizeof(ReplicationState);

//MemSet(replication_states, 0, ReplicationOriginShmemSize());

for (i = 0; i < max_replication_slots; i++)
LWLockInitialize(&replication_states[i].lock,
replication_states_ctl->tranche_id);
}
So I think it’s easier for understanding code.
What do you think?

That's imo just more work to maintain if additional fields added.

- Andres

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

On Wed, Apr 12, 2017 at 12:20 AM, Andres Freund <andres@anarazel.de> wrote:

On 2017-04-10 15:38:56 +0900, Michael Paquier wrote:

diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index 5eaf863e02..0aa468789c 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -473,7 +473,8 @@ ReplicationOriginShmemInit(void)

replication_states_ctl->tranche_id = LWTRANCHE_REPLICATION_ORIGIN;

-             MemSet(replication_states, 0, ReplicationOriginShmemSize());
+             MemSet(replication_states, 0,
+                        mul_size(max_replication_slots, sizeof(ReplicationState)));

What's the benefit of using mul_size here? That's usually only
beneficial in the original size computation - during use/initialization
an actual error should be impossible.

Clarity in initializing only the replication states.

To me the right fix seems to be to just do:
-               replication_states_ctl->tranche_id = LWTRANCHE_REPLICATION_ORIGIN;
-
-               MemSet(replication_states, 0, ReplicationOriginShmemSize());
+               MemSet(replication_states_ctl, 0, ReplicationOriginShmemSize());
+
+               replication_states_ctl->tranche_id = LWTRANCHE_REPLICATION_ORIGIN;

No?

That's what Bret is proposing. I am fine either way.
--
Michael

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

Hi Andres,

Yes, your solution is same with my. I have attached the patch file in former email.
Sorry for my mistake for this code. This code is come from 9.5.3. after that ,I create a patch file using 9.6.2.
The attachment is the email.

if (!found)
{
int i;
MemSet(replication_states_ctl, 0, ReplicationOriginShmemSize());
replication_states_ctl->tranche_id = LWLockNewTrancheId();
replication_states_ctl->tranche.name = "ReplicationOrigins";
replication_states_ctl->tranche.array_base =
&replication_states[0].lock;
replication_states_ctl->tranche.array_stride =
sizeof(ReplicationState);

//MemSet(replication_states, 0, ReplicationOriginShmemSize());

for (i = 0; i < max_replication_slots; i++)
LWLockInitialize(&replication_states[i].lock,
replication_states_ctl->tranche_id);
}

Thanks
Bret

发送自 Windows 10 版邮件<https://go.microsoft.com/fwlink/?LinkId=550986&gt;应用

发件人: Andres Freund<mailto:andres@anarazel.de>
发送时间: 2017年4月11日 23:22
收件人: shao bret<mailto:bret.shao@outlook.com>
抄送: Michael Paquier<mailto:michael.paquier@gmail.com>; PostgreSQL mailing lists<mailto:pgsql-bugs@postgresql.org>
主题: Re: [BUGS] 答复: [BUGS] BUG #14615: ReplicationOriginShmemInit Memory access cross-border

On 2017-04-10 07:02:06 +0000, shao bret wrote:

Hi Michael,
Thanks for your quickly response!
I think maybe you have a little misunderstanding with my solution.

My solution is that
if (!found)
{
int i;
MemSet(replication_states_ctl, 0, ReplicationOriginShmemSize());
replication_states_ctl->tranche_id = LWLockNewTrancheId();
replication_states_ctl->tranche.name = "ReplicationOrigins";
replication_states_ctl->tranche.array_base =
&replication_states[0].lock;
replication_states_ctl->tranche.array_stride =
sizeof(ReplicationState);

//MemSet(replication_states, 0, ReplicationOriginShmemSize());

for (i = 0; i < max_replication_slots; i++)
LWLockInitialize(&replication_states[i].lock,
replication_states_ctl->tranche_id);
}
So I think it’s easier for understanding code.
What do you think?

That's imo just more work to maintain if additional fields added.

- Andres