Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH 0/2] replication: fix uninitialized replica_version_id access
@ 2021-08-30 10:33 Serge Petrenko via Tarantool-patches
  2021-08-30 10:33 ` [Tarantool-patches] [PATCH 1/2] box: remove unused variable in process_register() Serge Petrenko via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-08-30 10:33 UTC (permalink / raw)
  To: v.shpilevoy, gorcunov; +Cc: tarantool-patches

https://github.com/tarantool/tarantool/tree/sp/version-uninitialized-access

Serge Petrenko (2):
  box: remove unused variable in process_register()
  box: fix uninitialized access to version_id in process_subscribe()

 src/box/box.cc | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

-- 
2.30.1 (Apple Git-130)


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Tarantool-patches] [PATCH 1/2] box: remove unused variable in process_register()
  2021-08-30 10:33 [Tarantool-patches] [PATCH 0/2] replication: fix uninitialized replica_version_id access Serge Petrenko via Tarantool-patches
@ 2021-08-30 10:33 ` Serge Petrenko via Tarantool-patches
  2021-08-30 10:33 ` [Tarantool-patches] [PATCH 2/2] box: fix uninitialized access to version_id in process_subscribe() Serge Petrenko via Tarantool-patches
  2021-08-30 14:55 ` [Tarantool-patches] [PATCH 0/2] replication: fix uninitialized replica_version_id access Cyrill Gorcunov via Tarantool-patches
  2 siblings, 0 replies; 7+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-08-30 10:33 UTC (permalink / raw)
  To: v.shpilevoy, gorcunov; +Cc: tarantool-patches

replica_version_id is unused in box_process_register,
so no point in filling it with anything coming from a remote instance.
Besides, version id isn't sent in register at all.

Follow-up #6034
---
 src/box/box.cc | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 0b12b1328..2c8113cbb 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2616,9 +2616,7 @@ box_process_register(struct ev_io *io, struct xrow_header *header)
 
 	struct tt_uuid instance_uuid = uuid_nil;
 	struct vclock replica_vclock;
-	uint32_t replica_version_id;
-	xrow_decode_register_xc(header, &instance_uuid, &replica_vclock,
-				&replica_version_id);
+	xrow_decode_register_xc(header, &instance_uuid, &replica_vclock, NULL);
 
 	if (!is_box_configured)
 		tnt_raise(ClientError, ER_LOADING);
-- 
2.30.1 (Apple Git-130)


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Tarantool-patches] [PATCH 2/2] box: fix uninitialized access to version_id in process_subscribe()
  2021-08-30 10:33 [Tarantool-patches] [PATCH 0/2] replication: fix uninitialized replica_version_id access Serge Petrenko via Tarantool-patches
  2021-08-30 10:33 ` [Tarantool-patches] [PATCH 1/2] box: remove unused variable in process_register() Serge Petrenko via Tarantool-patches
@ 2021-08-30 10:33 ` Serge Petrenko via Tarantool-patches
  2021-08-30 21:38   ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-30 14:55 ` [Tarantool-patches] [PATCH 0/2] replication: fix uninitialized replica_version_id access Cyrill Gorcunov via Tarantool-patches
  2 siblings, 1 reply; 7+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-08-30 10:33 UTC (permalink / raw)
  To: v.shpilevoy, gorcunov; +Cc: tarantool-patches

version_id might be left uninitialized if replica doesn't send it in a
SUBSCRIBE request.

This could lead to unpredictable behaviour: for example, master would
randomly choose between sending and not sending Raft state to the
replica.

We were safe until now, because replicas send their version in subscribe
request since at least version 1.7.5.

Try not to depend on replica sending us its version, better always be
safe.

Follow-up #6034
---
 src/box/box.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 2c8113cbb..f98437d05 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2850,8 +2850,8 @@ box_process_subscribe(struct ev_io *io, struct xrow_header *header)
 	struct tt_uuid replica_uuid = uuid_nil;
 	struct tt_uuid peer_replicaset_uuid = uuid_nil;
 	struct vclock replica_clock;
-	uint32_t replica_version_id;
 	vclock_create(&replica_clock);
+	uint32_t replica_version_id = 0;
 	bool anon;
 	uint32_t id_filter;
 	xrow_decode_subscribe_xc(header, &peer_replicaset_uuid, &replica_uuid,
-- 
2.30.1 (Apple Git-130)


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH 0/2] replication: fix uninitialized replica_version_id access
  2021-08-30 10:33 [Tarantool-patches] [PATCH 0/2] replication: fix uninitialized replica_version_id access Serge Petrenko via Tarantool-patches
  2021-08-30 10:33 ` [Tarantool-patches] [PATCH 1/2] box: remove unused variable in process_register() Serge Petrenko via Tarantool-patches
  2021-08-30 10:33 ` [Tarantool-patches] [PATCH 2/2] box: fix uninitialized access to version_id in process_subscribe() Serge Petrenko via Tarantool-patches
@ 2021-08-30 14:55 ` Cyrill Gorcunov via Tarantool-patches
  2 siblings, 0 replies; 7+ messages in thread
From: Cyrill Gorcunov via Tarantool-patches @ 2021-08-30 14:55 UTC (permalink / raw)
  To: Serge Petrenko; +Cc: v.shpilevoy, tarantool-patches

On Mon, Aug 30, 2021 at 01:33:46PM +0300, Serge Petrenko wrote:
> https://github.com/tarantool/tarantool/tree/sp/version-uninitialized-access
> 

Ack

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/2] box: fix uninitialized access to version_id in process_subscribe()
  2021-08-30 10:33 ` [Tarantool-patches] [PATCH 2/2] box: fix uninitialized access to version_id in process_subscribe() Serge Petrenko via Tarantool-patches
@ 2021-08-30 21:38   ` Vladislav Shpilevoy via Tarantool-patches
  2021-08-31  9:17     ` Serge Petrenko via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-08-30 21:38 UTC (permalink / raw)
  To: Serge Petrenko, gorcunov; +Cc: tarantool-patches

Hi! Thanks for the patch!

> diff --git a/src/box/box.cc b/src/box/box.cc
> index 2c8113cbb..f98437d05 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -2850,8 +2850,8 @@ box_process_subscribe(struct ev_io *io, struct xrow_header *header)
>  	struct tt_uuid replica_uuid = uuid_nil;
>  	struct tt_uuid peer_replicaset_uuid = uuid_nil;
>  	struct vclock replica_clock;
> -	uint32_t replica_version_id;
>  	vclock_create(&replica_clock);
> +	uint32_t replica_version_id = 0;

There seems to be an inconsistency in xrow_decode_subscribe(). It takes
multiple optional parameters, but only some of them are reset to default
values before the body is decoded.

- replicaset_uuid, instance_uuid, vclock, version_id are left untouched
if not found.

- anon, id_filter are nullified in the beginning.

Is there a reason why all the parameters can't be set to defaults / reset
right in xrow_decode_subscribe() before the body is decoded?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/2] box: fix uninitialized access to version_id in process_subscribe()
  2021-08-30 21:38   ` Vladislav Shpilevoy via Tarantool-patches
@ 2021-08-31  9:17     ` Serge Petrenko via Tarantool-patches
  2021-09-01 21:35       ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 7+ messages in thread
From: Serge Petrenko via Tarantool-patches @ 2021-08-31  9:17 UTC (permalink / raw)
  To: Vladislav Shpilevoy, gorcunov; +Cc: tarantool-patches



31.08.2021 00:38, Vladislav Shpilevoy пишет:
> Hi! Thanks for the patch!
>
>> diff --git a/src/box/box.cc b/src/box/box.cc
>> index 2c8113cbb..f98437d05 100644
>> --- a/src/box/box.cc
>> +++ b/src/box/box.cc
>> @@ -2850,8 +2850,8 @@ box_process_subscribe(struct ev_io *io, struct xrow_header *header)
>>   	struct tt_uuid replica_uuid = uuid_nil;
>>   	struct tt_uuid peer_replicaset_uuid = uuid_nil;
>>   	struct vclock replica_clock;
>> -	uint32_t replica_version_id;
>>   	vclock_create(&replica_clock);
>> +	uint32_t replica_version_id = 0;
> There seems to be an inconsistency in xrow_decode_subscribe(). It takes
> multiple optional parameters, but only some of them are reset to default
> values before the body is decoded.
>
> - replicaset_uuid, instance_uuid, vclock, version_id are left untouched
> if not found.
>
> - anon, id_filter are nullified in the beginning.
>
> Is there a reason why all the parameters can't be set to defaults / reset
> right in xrow_decode_subscribe() before the body is decoded?
I think not. Would you prefer nullifying all the parameters in
xrow_decode_subscribe() body?

-- 
Serge Petrenko


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/2] box: fix uninitialized access to version_id in process_subscribe()
  2021-08-31  9:17     ` Serge Petrenko via Tarantool-patches
@ 2021-09-01 21:35       ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-09-01 21:35 UTC (permalink / raw)
  To: Serge Petrenko, gorcunov; +Cc: tarantool-patches

On 31.08.2021 11:17, Serge Petrenko wrote:
> 
> 
> 31.08.2021 00:38, Vladislav Shpilevoy пишет:
>> Hi! Thanks for the patch!
>>
>>> diff --git a/src/box/box.cc b/src/box/box.cc
>>> index 2c8113cbb..f98437d05 100644
>>> --- a/src/box/box.cc
>>> +++ b/src/box/box.cc
>>> @@ -2850,8 +2850,8 @@ box_process_subscribe(struct ev_io *io, struct xrow_header *header)
>>>       struct tt_uuid replica_uuid = uuid_nil;
>>>       struct tt_uuid peer_replicaset_uuid = uuid_nil;
>>>       struct vclock replica_clock;
>>> -    uint32_t replica_version_id;
>>>       vclock_create(&replica_clock);
>>> +    uint32_t replica_version_id = 0;
>> There seems to be an inconsistency in xrow_decode_subscribe(). It takes
>> multiple optional parameters, but only some of them are reset to default
>> values before the body is decoded.
>>
>> - replicaset_uuid, instance_uuid, vclock, version_id are left untouched
>> if not found.
>>
>> - anon, id_filter are nullified in the beginning.
>>
>> Is there a reason why all the parameters can't be set to defaults / reset
>> right in xrow_decode_subscribe() before the body is decoded?
> I think not. Would you prefer nullifying all the parameters in
> xrow_decode_subscribe() body?

Yes, it would be more reliable I think.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-09-01 21:35 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-30 10:33 [Tarantool-patches] [PATCH 0/2] replication: fix uninitialized replica_version_id access Serge Petrenko via Tarantool-patches
2021-08-30 10:33 ` [Tarantool-patches] [PATCH 1/2] box: remove unused variable in process_register() Serge Petrenko via Tarantool-patches
2021-08-30 10:33 ` [Tarantool-patches] [PATCH 2/2] box: fix uninitialized access to version_id in process_subscribe() Serge Petrenko via Tarantool-patches
2021-08-30 21:38   ` Vladislav Shpilevoy via Tarantool-patches
2021-08-31  9:17     ` Serge Petrenko via Tarantool-patches
2021-09-01 21:35       ` Vladislav Shpilevoy via Tarantool-patches
2021-08-30 14:55 ` [Tarantool-patches] [PATCH 0/2] replication: fix uninitialized replica_version_id access Cyrill Gorcunov via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox