Skip to content

Commit

Permalink
Fix panic when servers return a wrapped error with status OK
Browse files Browse the repository at this point in the history
When wrapping an error that has a gRPC status, we reuse the underlying status message field and put
the error message in there. If the returned gRPC status is nil, then there is a nil dereference of
the status Message field.

This change fixes this behavior by returning Unknown status and the error message when the wrapped
error has status OK.
  • Loading branch information
atollena committed Jun 23, 2023
1 parent f24b4c7 commit 9a2ba92
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 4 deletions.
25 changes: 21 additions & 4 deletions status/status.go
Expand Up @@ -77,11 +77,18 @@ func FromProto(s *spb.Status) *Status {
// FromError returns a Status representation of err.
//
// - If err was produced by this package or implements the method `GRPCStatus()
// *Status`, or if err wraps a type satisfying this, the appropriate Status is
// returned. For wrapped errors, the message returned contains the entire
// err.Error() text and not just the wrapped status.
// *Status` and `GRPCStatus()` does not return nil, or if err wraps a type
// satisfying this, the Status from `GRPCStatus()` is returned. For wrapped
// errors, the message returned contains the entire err.Error() text and not
// just the wrapped status. In that case, ok is true.
//
// - If err is nil, a Status is returned with codes.OK and no message.
// - If err is nil, a Status is returned with codes.OK and no message, and ok
// is true.
//
// - If err implements the method `GRPCStatus() *Status` and `GRPCStatus()`
// returns nil (which maps to Codes.OK), or if err wraps a type
// satisfying this, a Status is returned with codes.Unknown and err's
// Error() message, and ok is false.
//
// - Otherwise, err is an error not compatible with this package. In this
// case, a Status is returned with codes.Unknown and err's Error() message,
Expand All @@ -92,10 +99,20 @@ func FromError(err error) (s *Status, ok bool) {
}
type grpcstatus interface{ GRPCStatus() *Status }
if gs, ok := err.(grpcstatus); ok {
if gs.GRPCStatus() == nil {
// Error has a status of OK. There is no sensible behavior for this, so map it to Unknown and discard the
// status.
return New(codes.Unknown, err.Error()), false
}
return gs.GRPCStatus(), true
}
var gs grpcstatus
if errors.As(err, &gs) {
if gs.GRPCStatus() == nil {
// Error wraps an error that has an OK status. There is no sensible behavior for this, so map it to Unknown
// and discard the status.
return New(codes.Unknown, err.Error()), false
}
p := gs.GRPCStatus().Proto()
p.Message = err.Error()
return status.FromProto(p), true
Expand Down
27 changes: 27 additions & 0 deletions status/status_test.go
Expand Up @@ -202,6 +202,33 @@ func (s) TestFromErrorWrapped(t *testing.T) {
}
}

type customErrorNilStatus struct {
}

func (c customErrorNilStatus) Error() string {
return "test"
}

func (c customErrorNilStatus) GRPCStatus() *Status {
return nil
}

func (s) TestFromErrorImplementsInterfaceReturnsOKStatus(t *testing.T) {
err := customErrorNilStatus{}
s, ok := FromError(err)
if ok || s.Code() != codes.Unknown || s.Message() != err.Error() {
t.Fatalf("FromError(%v) = %v, %v; want <Code()=%s, Message()=%q, Err()!=nil>, true", err, s, ok, codes.Unknown, err.Error())
}
}

func (s) TestFromErrorImplementsInterfaceReturnsOKStatusWrapped(t *testing.T) {
err := fmt.Errorf("wrapping: %w", customErrorNilStatus{})
s, ok := FromError(err)
if ok || s.Code() != codes.Unknown || s.Message() != err.Error() {
t.Fatalf("FromError(%v) = %v, %v; want <Code()=%s, Message()=%q, Err()!=nil>, true", err, s, ok, codes.Unknown, err.Error())
}
}

func (s) TestFromErrorImplementsInterfaceWrapped(t *testing.T) {
const code, message = codes.Internal, "test description"
err := fmt.Errorf("wrapped error: %w", customError{Code: code, Message: message})
Expand Down

0 comments on commit 9a2ba92

Please sign in to comment.