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

Fixes error found by gofuzz and reports line-column numbers for lexer/parser errors #2914

Merged
merged 6 commits into from
Jan 22, 2019

Conversation

gitlw
Copy link

@gitlw gitlw commented Jan 18, 2019


This change is Reviewable

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.

:lgtm: Few comments. Also, avoid checking for line and column number in existing tests. Instead, create a few tests just for that.

Reviewed 7 of 7 files at r1.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @codexnull, @gitlw, @manishrjain, and @srfrog)


gql/parser_test.go, line 2729 at r1 (raw file):

	require.Error(t, err)
	require.Contains(t, err.Error(),
		"line 4 column 35: Unrecognized character in lexDirective: U+002C ','")

Let's not check for line and column here. If we modify the way we count a bit, we'd need to keep on modifying these tests.


lex/lexer.go, line 51 at r1 (raw file):

func (i Item) Errorf(format string, args ...interface{}) error {
	return fmt.Errorf("line %d column %d: "+format,
		append([]interface{}{i.line, i.column}, args...)...)

, i.line, i.column, args... should work. You don't need the append.


lex/lexer.go, line 188 at r1 (raw file):

		Typ: ItemError,
		Val: fmt.Sprintf("while lexing %v at line %d column %d: "+format,
			append([]interface{}{l.Input, l.Line, l.Column}, args...)...),

No need for append.


lex/lexer.go, line 329 at r1 (raw file):

// isEndOfLine returns true if the rune is a Linefeed or a Carriage return.
func isEndOfLine(r rune) bool {

Maybe consolidate this func being used across multiple pkgs into one lex pkg.

Copy link
Author

@gitlw gitlw 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 10 files reviewed, 4 unresolved discussions (waiting on @codexnull, @manishrjain, and @srfrog)


gql/parser_test.go, line 2729 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Let's not check for line and column here. If we modify the way we count a bit, we'd need to keep on modifying these tests.

Done.


lex/lexer.go, line 51 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

, i.line, i.column, args... should work. You don't need the append.

It does not work without append.


lex/lexer.go, line 188 at r1 (raw file):

It does not work without append.
It does not work without append.


lex/lexer.go, line 329 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Maybe consolidate this func being used across multiple pkgs into one lex pkg.

Done.

@gitlw gitlw merged commit 2129101 into master Jan 22, 2019
@gitlw gitlw deleted the gitlw/lexer_line_column branch January 22, 2019 00:51
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
martinmr pushed a commit that referenced this pull request Aug 7, 2019
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.

Input found by gofuzz can crash the alpha server
2 participants