Skip to content

Conversation

jiceathome
Copy link
Contributor

Added unit test for deserialization and unit test for router ingress.

Fixes #4482

…ath header.

Added unit test for deserialization and unit test for router ingress.
@jiceathome jiceathome requested a review from a team as a code owner March 12, 2024 13:25
@matzf
Copy link
Contributor

matzf commented Mar 12, 2024

This change is Reviewable

@jiceathome jiceathome requested a review from matzf March 12, 2024 13:25
@jiceathome jiceathome changed the title paths: Add check for hopfield count < 64 when deserializing a scion path paths: Add check for hopfield count <= 64 when deserializing a scion path Mar 12, 2024
Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 4 files at r2, all commit messages.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @jiceatscion)


pkg/slayers/path/scion/decoded.go line 52 at r2 (raw file):

	// the length of a scion header. Yet a path of more than 64 hops cannot be followed to
	// the end because CurrHF is only 6 bits long.
	if s.NumHops > MaxHops {

This check now only applies to the Decoded variant of the SCION path representation, but we mainly use the Raw one e.g. in the router. The cleanest way to check in both cases, is to move this (back) to Base.


router/dataplane_test.go line 646 at r2 (raw file):

				dpath.Base.PathMeta.SegLen = [3]uint8{24, 24, 17}
				dpath.Base.NumINF = 3
				dpath.Base.NumHops = 65

As discussed, this seems to result in a broken path (possibly because only one info field is written).

Copy link
Contributor Author

@jiceathome jiceathome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @matzf)


pkg/slayers/path/scion/decoded.go line 52 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

This check now only applies to the Decoded variant of the SCION path representation, but we mainly use the Raw one e.g. in the router. The cleanest way to check in both cases, is to move this (back) to Base.

Done.


router/dataplane_test.go line 646 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

As discussed, this seems to result in a broken path (possibly because only one info field is written).

done

Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jiceatscion)


pkg/slayers/path/scion/raw_test.go line 91 at r3 (raw file):

	assert.NoError(t, overlongPath.SerializeTo(b)) // permitted, if only to enable this test.
	s := &scion.Raw{}
	assert.Error(t, s.DecodeFromBytes(b)) // invalid raw packet.

Nit: Maybe also add the assert.ErrorIs check here to ensure we get the expected error and that the path is not broken in some other way.


router/dataplane_test.go line 670 at r3 (raw file):

				expected := serrors.New("NumHops too large",
					"NumHops", 65, "Maximum", scion.MaxHops).Error()
				return assert.Equal(t, expected, err.Error())

Nit: "serrors" can be compared with Is, and so assert.ErrorIs(t, serrors.New("...",...), err) can be used to shorten this.

Suggestion:

				expected := serrors.New("NumHops too large",
					"NumHops", 65, "Maximum", scion.MaxHops)
				return assert.ErrorIs(t, expected, err)

Also make the similar check in dataplane_test.go in the same style.
Copy link
Contributor Author

@jiceathome jiceathome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 6 files reviewed, all discussions resolved (waiting on @matzf)


pkg/slayers/path/scion/raw_test.go line 91 at r3 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit: Maybe also add the assert.ErrorIs check here to ensure we get the expected error and that the path is not broken in some other way.

done


router/dataplane_test.go line 670 at r3 (raw file):

Previously, matzf (Matthias Frei) wrote…

Nit: "serrors" can be compared with Is, and so assert.ErrorIs(t, serrors.New("...",...), err) can be used to shorten this.

It is indeed nicer. Unfortunately we just found that a bug in serror prevents doing this. Filed # #4486

@jiceathome jiceathome enabled auto-merge (squash) March 18, 2024 11:49
Copy link
Contributor

@matzf matzf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)

@jiceathome jiceathome merged commit 61bf784 into scionproto:master Mar 18, 2024
@jiceathome jiceathome deleted the b4482 branch March 18, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question about the router: bound-checking for hop fields possibly missing
2 participants