Обсуждение: [BUGS] BUG #14615: ReplicationOriginShmemInit Memory access cross-border

The following bug has been logged on the website:

Bug reference:      14615
Logged by:          bret shao
Email address:      bret.shao@outlook.com
PostgreSQL version: 9.6.2
Operating system:   linux
Description:

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.


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

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

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

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

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