Added Nil Check for Package in File Descriptor #3223

tmulkern · 2023-07-21T12:19:46Z

What?

This code is to guard against a runtime error: invalid memory address or nil pointer dereference occuring

Why?

The package specifier is optional in protobuf spec https://protobuf.dev/programming-guides/proto3/#packages

Checklist

I have performed a self-review of my code.
I have added tests for my changes. N/A
I have run linter locally (make ci-like-lint) and all checks pass. N/A
I have run tests locally (make tests) and all tests pass. N/A
I have commented on my code, particularly in hard-to-understand areas. N/A

Related PR(s)/Issue(s)

Fixes issue #3222

The package specifier is optional in protobuf spec https://protobuf.dev/programming-guides/proto3/#packages This code is to guard against this

CLAassistant · 2023-07-21T12:19:52Z

All committers have signed the CLA.

mstoykov

LGTM!

Sorry for the slow review 🙇 .

Due to vacations across the team and the automatic assigning not taking this into account, sometimes things get assigned to people who are on vacation :(.

Unfortunately we are also at the end of the development cycle and have some higher priorities things we want to make certain do work for v0.46.0.

The fix seems pretty safe and I can't see any problems with it, but I still prefer if @olegbespalov gives it a 👍 specifically. This might mean that we do merge this in the codefreeze, but given the size and the fact it is a bug that doesn't seem like that much of a problem.

@tmulkern Can you please add a small test that panics without this change though.

Also we will need forward/backport this to xk6-grpc.

I will move this back to v0.46.0 milestone for now, but I can't promise we will actually make it :(

Thanks for the bug report and fix 🙇 And once again sorry for the slow reply

tmulkern · 2023-07-27T15:07:57Z

@mstoykov no problem on the response, I am in the same situation myself in my day job, it is the time for Vacations!!

tmulkern · 2023-07-27T15:09:29Z

Regarding the test, no problem in creating, but do we have a GRPC Test Service/Mock Service that I can reference. I couldn't find any. I am not that familiar with the code base, so I could be blind to it ;-)

codebien

Hey @tmulkern,
thanks for your contribution. 🙇

Regarding the test, no problem in creating, but do we have a GRPC Test Service/Mock Service that I can reference

I guess it should be enough to add a new test case to the following test table. I think we should cover also the case with clashing names now that we are skipping the package.

k6/lib/netext/grpcext/conn_test.go

Line 160 in 3c006a5

func TestResolveFileDescriptors(t *testing.T) {

Added tests for following scenarios * Services with no package - "NoPackage" * Services with no package and duplicated - "NoPackageDeduplicateServices" * One services with a package, one without - "MixPackage" * Two services with a package (Duplicate), one without - "MixPackageDeduplicateServices" Also updated the TestResolveFileDescriptors to handle cases with no packages

tmulkern · 2023-07-31T13:00:43Z

Hey @codebien

Added tests for following scenarios

Services with no package - "NoPackage"
Services with no package and duplicated - "NoPackageDeduplicateServices"
One services with a package, one without - "MixPackage"
Two services with a package (Duplicate), one without - "MixPackageDeduplicateServices"

Also updated the TestResolveFileDescriptors to handle cases with no packages

codebien · 2023-08-01T07:42:42Z

lib/netext/grpcext/conn_test.go

+		{
+			name:                "NoPackage",
+			services:            []string{"Service1", "Service2"},
+			expectedDescriptors: 2,
+		},
+		{
+			name:                "NoPackageDeduplicateServices",
+			services:            []string{"Service1", "Service2", "Service1"},
+			expectedDescriptors: 2,
+		},
+		{
+			name:                "MixPackage",
+			pkgs:                []string{"mypkg1"},
+			services:            []string{"Service1", "Service2"},
+			expectedDescriptors: 2,
+		},
+		{
+			name:                "MixPackageDeduplicateServices",
+			pkgs:                []string{"mypkg1"},
+			services:            []string{"Service1", "Service2", "Service1"},
+			expectedDescriptors: 2,
+		},


I don't think it is clear enough in this way because we already use undefined when we want to reuse for all of them. We could use an empty string for defining a no package.

Suggested change

{

name: "NoPackage",

services: []string{"Service1", "Service2"},

expectedDescriptors: 2,

},

{

name: "NoPackageDeduplicateServices",

services: []string{"Service1", "Service2", "Service1"},

expectedDescriptors: 2,

},

{

name: "MixPackage",

pkgs: []string{"mypkg1"},

services: []string{"Service1", "Service2"},

expectedDescriptors: 2,

},

{

name: "MixPackageDeduplicateServices",

pkgs: []string{"mypkg1"},

services: []string{"Service1", "Service2", "Service1"},

expectedDescriptors: 2,

},

{

name: "NoPackage",

pkgs: []string{""},

services: []string{"Service1", "Service2"},

expectedDescriptors: 2,

},

{

name: "NoPackageDeduplicateServices",

pkgs: []string{""},

services: []string{"Service1", "Service2", "Service1"},

expectedDescriptors: 2,

},

{

name: "MixPackage",

pkgs: []string{"mypkg1", ""},

services: []string{"Service1", "Service2"},

expectedDescriptors: 2,

},

{

name: "MixPackageDeduplicateServices",

pkgs: []string{"mypkg1", ""},

services: []string{"Service1", "Service2", "Service1"},

expectedDescriptors: 2,

},

codebien · 2023-08-01T08:00:10Z

lib/netext/grpcext/conn_test.go

+			var pkg string
+			if len(tt.pkgs) == 1 {
+				pkg = tt.pkgs[0]
+			}
+


I'm for completely removing this, every service should have an equivalent package defined (empty or non-empty), in this way we can remove any eventual confusion.

olegbespalov

To be honest, in that particular case, I'm more up for keeping the logic (with the comment), like

pkg := ""

// if only one package is defined then
// the package is the same for every service
if len(tt.pkgs) == 1 {
   pkg = tt.pkgs[0]
}

The rationale is that having the empty strings confuses me more than the logic of the comment.

olegbespalov · 2023-08-07T08:30:01Z

lib/netext/grpcext/conn_test.go

 				// if only one package is defined then
 				// the package is the same for every service


Suggested change

// if only one package is defined then

// the package is the same for every service

tmulkern · 2023-08-08T15:43:40Z

Applogies people for the late reply, as mstoykov alluded to it is vacation season and I was away on vacation. I will look into coming up with a commit to the test based on your comments

olegbespalov · 2023-09-19T11:59:32Z

Hi @tmulkern !

I am sharing this here for the sake of transparency. We have another PR related to the support of gRPC's reflection v1.

I believe that PR also fixes the issue that you reported. So we will probably close this PR in favor of #3338.

Anyway, thank you for reporting and contribution! 🙇

tmulkern · 2023-09-19T15:59:02Z

Hey olegbespalov, thank's for the update. Sorry, life got in the way the 2 months and I hadn't got round to finishing this PR.

Added Nil Check for Package in File Descriptor

94c7532

The package specifier is optional in protobuf spec https://protobuf.dev/programming-guides/proto3/#packages This code is to guard against this

github-actions bot requested review from codebien and mstoykov July 21, 2023 12:19

mstoykov added this to the v0.47.0 milestone Jul 26, 2023

mstoykov linked an issue Jul 27, 2023 that may be closed by this pull request

GRPC reflection will crash when there is no package specifier defined #3222

Closed

mstoykov requested a review from olegbespalov July 27, 2023 07:43

mstoykov previously approved these changes Jul 27, 2023

View reviewed changes

mstoykov modified the milestones: v0.47.0, v0.46.0 Jul 27, 2023

codebien requested changes Jul 31, 2023

View reviewed changes

tmulkern dismissed mstoykov’s stale review via c84e30b July 31, 2023 12:59

mstoykov requested a review from codebien August 1, 2023 07:17

codebien requested changes Aug 1, 2023

View reviewed changes

olegbespalov approved these changes Aug 7, 2023

View reviewed changes

olegbespalov mentioned this pull request Aug 7, 2023

Backport "Added Nil Check for Package in File Descriptor" grafana/xk6-grpc#32

Closed

oleiade modified the milestones: v0.46.0, v0.47.0 Aug 14, 2023

tmulkern closed this Sep 19, 2023

codebien removed this from the v0.47.0 milestone Oct 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Nil Check for Package in File Descriptor #3223

Added Nil Check for Package in File Descriptor #3223

tmulkern commented Jul 21, 2023

CLAassistant commented Jul 21, 2023 •

edited

Loading

mstoykov left a comment

tmulkern commented Jul 27, 2023

tmulkern commented Jul 27, 2023

codebien left a comment •

edited

Loading

tmulkern commented Jul 31, 2023

codebien Aug 1, 2023

codebien Aug 1, 2023

olegbespalov left a comment

olegbespalov Aug 7, 2023

tmulkern commented Aug 8, 2023 •

edited

Loading

olegbespalov commented Sep 19, 2023

tmulkern commented Sep 19, 2023

		// if only one package is defined then
		// the package is the same for every service

Added Nil Check for Package in File Descriptor #3223

Added Nil Check for Package in File Descriptor #3223

Conversation

tmulkern commented Jul 21, 2023

What?

Why?

Checklist

Related PR(s)/Issue(s)

CLAassistant commented Jul 21, 2023 • edited Loading

mstoykov left a comment

Choose a reason for hiding this comment

tmulkern commented Jul 27, 2023

tmulkern commented Jul 27, 2023

codebien left a comment • edited Loading

Choose a reason for hiding this comment

tmulkern commented Jul 31, 2023

codebien Aug 1, 2023

Choose a reason for hiding this comment

codebien Aug 1, 2023

Choose a reason for hiding this comment

olegbespalov left a comment

Choose a reason for hiding this comment

olegbespalov Aug 7, 2023

Choose a reason for hiding this comment

tmulkern commented Aug 8, 2023 • edited Loading

olegbespalov commented Sep 19, 2023

tmulkern commented Sep 19, 2023

CLAassistant commented Jul 21, 2023 •

edited

Loading

codebien left a comment •

edited

Loading

tmulkern commented Aug 8, 2023 •

edited

Loading