From b33e1d2b9f1690c0003b536a71d6a38c48cee1aa Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Tue, 3 Jan 2023 13:39:46 +0000 Subject: [PATCH] bugfixes: fix 2 issues which could cause invalid ranges to be returned - When a range is shrinked from both ends, we would incorrectly send back INVALIDATE twice for the same range (the higher end). This was caused by improper usage of Go for .. range loops - When a range is changed to a window completely outside the room list (e.g 5 rooms, range=[10,15]) we would incorrectly send back SYNC 10,15 with a single room ID, the last room in the list. This was caused by boundary check code in SliceInto capping ranges to the last element if the index positions are > room list. However in this case, this causes the last room to be returned. With regression tests (UT and E2E) --- sync3/handler/connstate.go | 4 +- sync3/range.go | 3 + sync3/range_test.go | 9 ++- tests-e2e/lists_test.go | 127 ++++++++++++++++++++++++++++++++++++- 4 files changed, 139 insertions(+), 4 deletions(-) diff --git a/sync3/handler/connstate.go b/sync3/handler/connstate.go index 8ab51b2..dd9a18f 100644 --- a/sync3/handler/connstate.go +++ b/sync3/handler/connstate.go @@ -265,10 +265,10 @@ func (s *ConnState) onIncomingListRequest(ctx context.Context, builder *RoomsBui if len(removedRanges) > 0 { logger.Trace().Interface("range", removedRanges).Msg("INVALIDATEing because ranges were removed") } - for _, r := range removedRanges { + for i := range removedRanges { responseOperations = append(responseOperations, &sync3.ResponseOpRange{ Operation: sync3.OpInvalidate, - Range: r[:], + Range: removedRanges[i][:], }) } diff --git a/sync3/range.go b/sync3/range.go index 22db3e4..7033826 100644 --- a/sync3/range.go +++ b/sync3/range.go @@ -260,6 +260,9 @@ func (r SliceRanges) SliceInto(slice Subslicer) []Subslicer { // empty slices subslice into themselves continue } + if sr[0] >= sliceLen && sr[1] >= sliceLen { + continue + } if sr[0] >= sliceLen { sr[0] = sliceLen - 1 } diff --git a/sync3/range_test.go b/sync3/range_test.go index e1d427f..5a116c1 100644 --- a/sync3/range_test.go +++ b/sync3/range_test.go @@ -108,12 +108,19 @@ func TestRange(t *testing.T) { alphabet[0:1], alphabet[1:2], }, }, + { + input: SliceRanges([][2]int64{ + {30, 99}, + }), + want: [][]string{}, + }, } for _, tc := range testCases { result := tc.input.SliceInto(stringSlice(alphabet)) if len(result) != len(tc.want) { - t.Errorf("%+v subslice mismatch: got %v want %v", tc, len(result), len(tc.want)) + t.Errorf("%+v subslice mismatch: got %v want %v \ngot: %+v\nwant: %+v\n", tc, len(result), len(tc.want), result, tc.want) + continue } for i := range result { got := result[i].(stringSlice) diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index 4294059..4b40e07 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -713,5 +713,130 @@ func TestChangeSortOrder(t *testing.T) { m.MatchV3InvalidateOp(0, 20), m.MatchV3SyncOp(0, 20, []string{gotNameToIDs["Apple"], gotNameToIDs["Kiwi"], gotNameToIDs["Lemon"], gotNameToIDs["Orange"]}), ))) - +} + +// Regression test that a window can be shrunk and the INVALIDATE appears correctly for the low/high ends of the range +func TestShrinkRange(t *testing.T) { + alice := registerNewUser(t) + var roomIDs []string // most recent first + for i := 0; i < 10; i++ { + time.Sleep(time.Millisecond) // ensure creation timestamp changes + roomIDs = append([]string{alice.CreateRoom(t, map[string]interface{}{ + "preset": "public_chat", + "name": fmt.Sprintf("Room %d", i), + })}, roomIDs...) + } + res := alice.SlidingSync(t, sync3.Request{ + Lists: []sync3.RequestList{ + { + Ranges: sync3.SliceRanges{{0, 20}}, + }, + }, + }) + m.MatchResponse(t, res, m.MatchList(0, m.MatchV3Count(10), m.MatchV3Ops( + m.MatchV3SyncOp(0, 20, roomIDs), + ))) + // now shrink the window on both ends + res = alice.SlidingSync(t, sync3.Request{ + Lists: []sync3.RequestList{ + { + Ranges: sync3.SliceRanges{{2, 6}}, + }, + }, + }, WithPos(res.Pos)) + m.MatchResponse(t, res, m.MatchList(0, m.MatchV3Count(10), m.MatchV3Ops( + m.MatchV3InvalidateOp(0, 1), + m.MatchV3InvalidateOp(7, 20), + ))) +} + +// Regression test that you can expand a window without it causing problems. Previously, it would cause +// a spurious {"op":"SYNC","range":[11,20],"room_ids":["!jHVHDxEWqTIcbDYoah:synapse"]} op for a bad index +func TestExpandRange(t *testing.T) { + alice := registerNewUser(t) + var roomIDs []string // most recent first + for i := 0; i < 10; i++ { + time.Sleep(time.Millisecond) // ensure creation timestamp changes + roomIDs = append([]string{alice.CreateRoom(t, map[string]interface{}{ + "preset": "public_chat", + "name": fmt.Sprintf("Room %d", i), + })}, roomIDs...) + } + res := alice.SlidingSync(t, sync3.Request{ + Lists: []sync3.RequestList{ + { + Ranges: sync3.SliceRanges{{0, 10}}, + }, + }, + }) + m.MatchResponse(t, res, m.MatchList(0, m.MatchV3Count(10), m.MatchV3Ops( + m.MatchV3SyncOp(0, 10, roomIDs), + ))) + // now expand the window + res = alice.SlidingSync(t, sync3.Request{ + Lists: []sync3.RequestList{ + { + Ranges: sync3.SliceRanges{{0, 20}}, + }, + }, + }, WithPos(res.Pos)) + m.MatchResponse(t, res, m.MatchList(0, m.MatchV3Count(10), m.MatchV3Ops())) +} + +// Regression test for Element X which has 2 identical lists and then changes the ranges in weird ways, +// causing the proxy to send back bad data. The request data here matches what EX sent. +func TestMultipleSameList(t *testing.T) { + alice := registerNewUser(t) + // the account had 16 rooms, so do this now + var roomIDs []string // most recent first + for i := 0; i < 16; i++ { + time.Sleep(time.Millisecond) // ensure creation timestamp changes + roomIDs = append([]string{alice.CreateRoom(t, map[string]interface{}{ + "preset": "public_chat", + "name": fmt.Sprintf("Room %d", i), + })}, roomIDs...) + } + firstList := sync3.RequestList{ + Sort: []string{sync3.SortByRecency, sync3.SortByName}, + Ranges: sync3.SliceRanges{{0, 20}}, + RoomSubscription: sync3.RoomSubscription{ + RequiredState: [][2]string{{"m.room.avatar", ""}, {"m.room.encryption", ""}}, + TimelineLimit: 10, + }, + } + secondList := sync3.RequestList{ + Sort: []string{sync3.SortByRecency, sync3.SortByName}, + Ranges: sync3.SliceRanges{{0, 16}}, + RoomSubscription: sync3.RoomSubscription{ + RequiredState: [][2]string{{"m.room.avatar", ""}, {"m.room.encryption", ""}}, + }, + } + res := alice.SlidingSync(t, sync3.Request{ + Lists: []sync3.RequestList{ + firstList, secondList, + }, + }) + m.MatchResponse(t, res, + m.MatchList(0, m.MatchV3Count(16), m.MatchV3Ops( + m.MatchV3SyncOp(0, 20, roomIDs, false), + )), + m.MatchList(1, m.MatchV3Count(16), m.MatchV3Ops( + m.MatchV3SyncOp(0, 16, roomIDs, false), + )), + ) + // now change both list ranges in a valid but strange way, and get back bad responses + firstList.Ranges = sync3.SliceRanges{{2, 15}} // from [0,20] + secondList.Ranges = sync3.SliceRanges{{0, 20}} // from [0,16] + res = alice.SlidingSync(t, sync3.Request{ + Lists: []sync3.RequestList{ + firstList, secondList, + }, + }, WithPos(res.Pos)) + m.MatchResponse(t, res, + m.MatchList(0, m.MatchV3Count(16), m.MatchV3Ops( + m.MatchV3InvalidateOp(0, 1), + m.MatchV3InvalidateOp(16, 20), + )), + m.MatchList(1, m.MatchV3Count(16), m.MatchV3Ops()), + ) }