[Tarantool-patches] [PATCH 0/2] replication: fix uninitialized replica_version_id access

[Tarantool-patches] [PATCH 0/2] replication: fix uninitialized replica_version_id access

[Serge Petrenko via 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)

[Tarantool-patches] [PATCH 1/2] box: remove unused variable in process_register()

[Serge Petrenko via 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)

[Tarantool-patches] [PATCH 2/2] box: fix uninitialized access to version_id in process_subscribe()

[Serge Petrenko via 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)

Re: [Tarantool-patches] [PATCH 0/2] replication: fix uninitialized replica_version_id access

[Cyrill Gorcunov via 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

Re: [Tarantool-patches] [PATCH 2/2] box: fix uninitialized access to version_id in process_subscribe()

[Vladislav Shpilevoy via 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?

Re: [Tarantool-patches] [PATCH 2/2] box: fix uninitialized access to version_id in process_subscribe()

[Serge Petrenko via 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

Re: [Tarantool-patches] [PATCH 2/2] box: fix uninitialized access to version_id in process_subscribe()

[Vladislav Shpilevoy via 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.