SIGSEGV on empty grpc findTraces query

[FauxFaux]

When processing a request for findTraces with no parameters, the server experiences a segmentation fault/segfault.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1232df8]

goroutine 29502 [running]:
github.com/jaegertracing/jaeger/cmd/query/app.(*GRPCHandler).FindTraces(0xc006f19060, 0xc006c303c0, 0x1e2bb20, 0xc006c710c0, 0xc006f19060, 0x20)
	github.com/jaegertracing/jaeger/cmd/query/app/grpc_handler.go:88 +0x98
github.com/jaegertracing/jaeger/proto-gen/api_v2._QueryService_FindTraces_Handler(0x14833a0, 0xc006f19060, 0x1e292a0, 0xc006c76000, 0x26d5680, 0xc000274000)
	github.com/jaegertracing/jaeger/proto-gen/api_v2/query.pb.go:994 +0x10b
google.golang.org/grpc.(*Server).processStreamingRPC(0xc00023c340, 0x1e2f7e0, 0xc0004a8300, 0xc000274000, 0xc006f226f0, 0x2687aa0, 0x0, 0x0, 0x0)
	google.golang.org/grpc@v1.29.1/server.go:1329 +0xcd8
google.golang.org/grpc.(*Server).handleStream(0xc00023c340, 0x1e2f7e0, 0xc0004a8300, 0xc000274000, 0x0)
	google.golang.org/grpc@v1.29.1/server.go:1409 +0xc5c
google.golang.org/grpc.(*Server).serveStreams.func1.1(0xc0004c5630, 0xc00023c340, 0x1e2f7e0, 0xc0004a8300, 0xc000274000)
	google.golang.org/grpc@v1.29.1/server.go:746 +0xa5
created by google.golang.org/grpc.(*Server).serveStreams.func1
	google.golang.org/grpc@v1.29.1/server.go:744 +0xa5

To Reproduce

Using the grpc-js generated client from ts-proto, QueryServiceClient: qsc.findTraces({}), i.e. make a request with none of the required parameters populated.

Expected behavior
An error to be returned, and the server to not be down / restart.

Version:

• OS: Ubuntu
• Jaeger version: jaegertracing/all-in-one:1.22
• Deployment: docker-compose

[yurishkuro]

func (g *GRPCHandler) FindTraces(r *api_v2.FindTracesRequest, stream api_v2.QueryService_FindTracesServer) error

It's very strange that gRPC allows the above handler to be called with r == nil, but I guess it's what we get when everything in the IDL is optional.

[yurishkuro]

I think this would also happen with all other API methods since most of them require a request object which could be nil, so the code need to check for that and return an error.

[jeffy-mathew]

Hi, I would like to work on this issue

[yurishkuro]

It's yours

[jpkrohling]

Wasn't this fixed by #2979 ?

[yurishkuro]

Wasn't this fixed by #2979 ?

Only FindTraces method was fixed there, we should add this check to all methods.

[akuzni2]

/assign
I'd like to take this up :)

[albertteoh]

Go for it @akuzni2 :)

[akuzni2]

Have a bit of a question on this - should we be checking if the actual client function *Request instances coming into the client are nil or just if any of the exported struct fields might be nil / default struct?

Example:
FindTraces client function requires a r *api_v2.FindTracesRequest argument where FindTracesRequest has a Query *TraceQueryParameters struct field. Should we be verifying that both FindTracesRequest and Query are not nil?

[yurishkuro]

yes

[akuzni2]

I did create a PR for this issue - had a few questions that I left on the PR submissions. Thanks!