Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a check to throw an error is a nil pointer is passed to unmarshalOrCopy #5334

Merged
merged 3 commits into from
Apr 30, 2020

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Apr 29, 2020


This change is Reviewable

I am not sure how this happens because all I have is the stacktrace but
it looks like unmarshalOrCopy is getting called with a nil plist. I am
not sure how this happens because l.plist is being instantiated
correctly. This PR is doing two things to fix this issue.

1. Create the posting list right before calling unmarshalOrCopy to make
   sure that a nil value is never passed.
2. Add a check to throw an error is a nil pointer is passed to
   unmarshalOrCopy.

Fixes DGRAPH-1300
@martinmr martinmr requested a review from manishrjain as a code owner April 29, 2020 21:59
posting/mvcc.go Outdated Show resolved Hide resolved
Copy link
Contributor

@parasssh parasssh left a comment

Choose a reason for hiding this comment

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

minor comment.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

This one looks like the system didn't have enough memory to do the posting list unmarshal. So, better to check with on Go slack and see what they think.

:lgtm: to putting the error checking, remove all other changes.

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @martinmr and @parasssh)


posting/mvcc.go, line 233 at r1 (raw file):

Previously, parasssh wrote…

object -> posting list

I'd make the %s the last thing. Because hex.Dump can take multiple lines.

Maybe just do this, and change nothing else.


posting/mvcc.go, line 268 at r2 (raw file):

	}

	l := &List{}

Avoid unnecessary changes.


posting/mvcc.go, line 297 at r2 (raw file):

			return l, nil
		case BitCompletePosting:
			pl := &pb.PostingList{}

No need for this.

Copy link
Contributor Author

@martinmr martinmr 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 1 files reviewed, 3 unresolved discussions (waiting on @manishrjain and @parasssh)


posting/mvcc.go, line 233 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I'd make the %s the last thing. Because hex.Dump can take multiple lines.

Maybe just do this, and change nothing else.

Done.


posting/mvcc.go, line 268 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Avoid unnecessary changes.

Done.


posting/mvcc.go, line 297 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

No need for this.

Done.

@martinmr martinmr changed the title Fix SIGSEV in mvcc.go reported by Sentry. Add a check to throw an error is a nil pointer is passed to unmarshalOrCopy Apr 30, 2020
@martinmr martinmr requested a review from manishrjain April 30, 2020 18:17
@martinmr martinmr merged commit 63a5e3a into master Apr 30, 2020
@martinmr martinmr deleted the martinmr/fix-sentry branch April 30, 2020 22:54
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants