Fix panic on logVerifier.VerifyRoot when newRoot is nil #779

AMarcedone · 2017-08-05T03:50:04Z

Solves #778

…rotobuf fields.

gdbelvin · 2017-08-07T11:33:59Z

client/verifier.go

-	if err := tcrypto.Verify(c.pubKey, hash, newRoot.Signature); err != nil {
-		return err
+	if (newRoot == nil){
+		return fmt.Errorf("VerifyRoot() error: newRoot == nil.")


Error strings should not be capitalized.

I'd remove the "." at the end too.

Actually, sorry for the back and forth here, but since the capitalized string is actually the method name, what you had originally is correct.

We don't capitalize sentences in errors because error chaining is a common practice, so it would look weird, but in this case it works fine.

I'd go with a small change, which is dropping the " error" part: fmt.Errorf("VerifyRoot(): newRoot == nil")

gdbelvin · 2017-08-07T11:34:07Z

client/verifier.go

-		return err
+	if (newRoot == nil){
+		return fmt.Errorf("VerifyRoot() error: newRoot == nil.")
+	}else {


This block can be outdented

Not sure what you mean. Do you want me to take the code out of the else branch, as the else is redundant since when the if branch is taken the function returns?
I wrote it like this as I think it is easier to see that in the else branch newRoot cannot be null, but happy to switch

@gdbelvin I made the switch. Please let me know if this is what you meant.

Not Gary, but it looks good. :)

Just clarifying, it's somewhat of a general Go-style rule that, if you return inside the if, you omit the else clause.

Eg, we prefer:

if something { return fmt.Errorf("...") } // code continues here

instead of

if something { return fmt.Errorf("...") } else { // code continues here }

gdbelvin

Looks good. Just a few code style comments.

codingllama

Hey Antonio, thanks for the contributions.

Could you please add a test for VerifyRoot where newRoot is nil?

codingllama · 2017-08-07T11:45:39Z

AUTHORS

@@ -8,5 +8,6 @@
 #
 # Please keep the list sorted.

+Antonio Marcedone <a.marcedone@gmail.com>


We should add you to contributors, but not authors. It has a different meaning.

From the contributors file:

# When adding J Random Contributor's name to this file, # either J's name or J's organization's name should be # added to the AUTHORS file, depending on whether the # individual or corporate CLA was used.

I signed the individual CLA.

OK. What got me is that, apparently, we're missing a bunch of people at AUTHORS, so I understood their absence as a sign that we shouldn't do that. A glimpse at another repo quickly showed my error.

Edit: expanded explanation.

codingllama · 2017-08-07T11:46:21Z

client/verifier.go

@@ -22,6 +22,7 @@ import (
 	tcrypto "github.com/google/trillian/crypto"
 	"github.com/google/trillian/merkle"
 	"github.com/google/trillian/merkle/hashers"
+	"fmt"


nit: Please move "fmt" to the block of Golang imports (below crypto/sha256)

codingllama · 2017-08-07T11:47:02Z

client/verifier.go

-	hash := tcrypto.HashLogRoot(*newRoot)
-	if err := tcrypto.Verify(c.pubKey, hash, newRoot.Signature); err != nil {
-		return err
+	if (newRoot == nil){


nit: Parenthesis aren't usually in Go ifs (unless really needed)

codingllama · 2017-08-07T11:47:32Z

client/verifier.go

-	if err := tcrypto.Verify(c.pubKey, hash, newRoot.Signature); err != nil {
-		return err
+	if (newRoot == nil){
+		return fmt.Errorf("VerifyRoot() error: newRoot == nil.")


I'd remove the "." at the end too.

codingllama · 2017-08-07T11:48:13Z

client/verifier.go

 	}

 	// Implicitly trust the first root we get.
-	if trusted.TreeSize != 0 {
+	if trusted.GetTreeSize() != 0 {


Is there a reason for the change on the getters here and below?

Yes. If trusted == nil, trusted.TreeSize is a nil pointer dereference panic which causes SEGFAULT when tested. The getter prevents this (if you look at the code, it just adds a nil test).

Calling VerifyRoot with trusted = nil sounds like a programming error to me. I think we should error out earlier, just as we do if newRoot == nil.

@gdbelvin, what say you?

I would agree. Clients should explicitly create an initial trusted root to work with.
This can easily be a static object with TreeSize == 0 and RootHash = empty root

codingllama · 2017-08-07T11:48:38Z

crypto/data_formats.go

-		mapKeyRootHash:       base64.StdEncoding.EncodeToString(root.RootHash),
-		mapKeyTimestampNanos: strconv.FormatInt(root.TimestampNanos, 10),
-		mapKeyTreeSize:       strconv.FormatInt(root.TreeSize, 10)}
+		mapKeyRootHash:       base64.StdEncoding.EncodeToString(root.GetRootHash()),


Is there a reason for the changes here?

As above, getters prevent nil pointer dereference errors.

root is not a pointer, you can't really pass nil to this function. (Maybe it should be a pointer, but that's another discussion.)

codingllama · 2017-08-07T11:54:48Z

Could you edit the PR's title as well, it's too generic and will be ambiguous once in the Git log. I suggest something like "Fix panic on logVerifier.VerifyRoot when newRoot is nil"

…reference errors/segfaults. Other minor nits in response to PR reviews.

AMarcedone · 2017-08-07T22:50:00Z

@gdbelvin and @codingllama thanks a lot for your reviews!
I made the requested changes. PTAL

codingllama · 2017-08-08T10:15:47Z

AUTHORS

@@ -8,5 +8,6 @@
 #
 # Please keep the list sorted.

+Antonio Marcedone <a.marcedone@gmail.com>


OK. What got me is that, apparently, we're missing a bunch of people at AUTHORS, so I understood their absence as a sign that we shouldn't do that. A glimpse at another repo quickly showed my error.

Edit: expanded explanation.

codingllama · 2017-08-08T10:23:05Z

client/client_test.go

@@ -254,3 +257,42 @@ func TestUpdateRoot(t *testing.T) {
 		t.Errorf("Tree size after add Leaf: %v, want > %v", got, want)
 	}
 }
+


Could we move the tests to verifier_test.go?

codingllama · 2017-08-08T10:27:10Z

client/client_test.go

@@ -254,3 +257,42 @@ func TestUpdateRoot(t *testing.T) {
 		t.Errorf("Tree size after add Leaf: %v, want > %v", got, want)
 	}
 }
+
+func TestVerifyRootReturnsErrWhenNewRootIsNil(t *testing.T) {


go style nit: Could we structure both tests as a single, table-driven test?

func TestVerifyRootErrors(t *testing.T) { tests := []struct { desc string trusted, newRoot *trillian.SignedLogRoot }{ {desc: "newRootNil", trusted: someRoot, newRoot: nil}, {desc: "trustedNil",trusted: nil, newRoot: someRoot}, // so on... } for _, test := range tests { if err := verifier.VerifyRoot(test.trusted, test.newRoot); err == nil { t.Errorf("%v: VerifyRoot() returned err = nil", test.desc) } } }

https://github.com/golang/go/wiki/TableDrivenTests

codingllama · 2017-08-08T10:27:29Z

client/verifier.go

@@ -18,6 +18,7 @@ import (
 	"crypto"
 	"crypto/sha256"

+	"fmt"


nit: Move to the block above

codingllama · 2017-08-08T10:28:53Z

client/verifier.go

 	}

 	// Implicitly trust the first root we get.
-	if trusted.TreeSize != 0 {
+	if trusted.GetTreeSize() != 0 {


Calling VerifyRoot with trusted = nil sounds like a programming error to me. I think we should error out earlier, just as we do if newRoot == nil.

@gdbelvin, what say you?

codingllama · 2017-08-08T10:31:26Z

client/verifier.go

@@ -68,14 +72,14 @@ func (c *logVerifier) VerifyRoot(trusted, newRoot *trillian.SignedLogRoot,
 // the currently trusted root. The inclusion proof must be requested for Root().TreeSize.
 func (c *logVerifier) VerifyInclusionAtIndex(trusted *trillian.SignedLogRoot, data []byte, leafIndex int64, proof [][]byte) error {
 	leaf := c.buildLeaf(data)
-	return c.v.VerifyInclusionProof(leafIndex, trusted.TreeSize,
-		proof, trusted.RootHash, leaf.MerkleLeafHash)
+	return c.v.VerifyInclusionProof(leafIndex, trusted.GetTreeSize(),


Ditto here and at VerifyInclusionByHash. I'd rather error out or panic, silently proceeding makes the trusted = nil misstep harder to detect.

Please add a test for VerifyInclusion* methods too, if you decide to touch them.

A panic (SEGFAULT) is bad for gobind integration (the android app hangs) and might cause errors too, so I would go for an error instead.

codingllama · 2017-08-08T10:33:48Z

crypto/data_formats.go

-		mapKeyRootHash:       base64.StdEncoding.EncodeToString(root.RootHash),
-		mapKeyTimestampNanos: strconv.FormatInt(root.TimestampNanos, 10),
-		mapKeyTreeSize:       strconv.FormatInt(root.TreeSize, 10)}
+		mapKeyRootHash:       base64.StdEncoding.EncodeToString(root.GetRootHash()),


root is not a pointer, you can't really pass nil to this function. (Maybe it should be a pointer, but that's another discussion.)

gdbelvin · 2017-08-08T21:45:17Z

That sounds like the right path. Thanks for taking this on Antonio

…

On Tue, Aug 8, 2017 at 10:41 PM Antonio Marcedone ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In client/verifier.go <#779 (comment)>: > @@ -68,14 +72,14 @@ func (c *logVerifier) VerifyRoot(trusted, newRoot *trillian.SignedLogRoot, // the currently trusted root. The inclusion proof must be requested for Root().TreeSize. func (c *logVerifier) VerifyInclusionAtIndex(trusted *trillian.SignedLogRoot, data []byte, leafIndex int64, proof [][]byte) error { leaf := c.buildLeaf(data) - return c.v.VerifyInclusionProof(leafIndex, trusted.TreeSize, - proof, trusted.RootHash, leaf.MerkleLeafHash) + return c.v.VerifyInclusionProof(leafIndex, trusted.GetTreeSize(), A panic (SEGFAULT) is bad for gobind integration (the android app hangs) and might cause errors too, so I would go for an error instead. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#779 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMHTni3UXEUZU6Cf9z6M16h1s7sn9qxks5sWNYYgaJpZM4OuWsN> .

…put.

AMarcedone · 2017-08-08T22:50:10Z

Updated after the most recent comments. And thanks for your patience, I am just starting to write in go for this project :)
PTAL

codingllama

Thanks for sticking with us, Antonio. I've a few more nits left and a comment asking for auto-formatting, but I think we're close to a merge here.

codingllama · 2017-08-09T09:39:53Z

client/verifier_test.go

+	"github.com/google/trillian/crypto/keys"
+	"github.com/google/trillian/merkle/rfc6962"
+	"github.com/google/trillian/testonly"
+	"strings"


nit: Move "strings" above "testing", on the Golang libraries block.

codingllama · 2017-08-09T09:42:40Z

client/verifier_test.go

+	logVerifier := NewLogVerifier(nil, nil)
+	err := logVerifier.VerifyInclusionAtIndex(nil, nil, 1, nil)
+	if err == nil {
+		t.Errorf("VerifyInclusionAtIndex() error expected, but none was thrown")


nit: Technically, Go errors aren't "thrown", that's Java/C++/etc lingo. ;)

Ditto below.

codingllama · 2017-08-09T09:44:27Z

client/verifier_test.go

+	if err == nil {
+		t.Errorf("VerifyInclusionAtIndex() error expected, but none was thrown")
+	}
+	if !strings.Contains(err.Error(), "trusted == nil"){


I'd rather drop the error string check and call VerifyInclusionAtIndex with only trusted = nil, so it's clear what the source of the error is. Error message checking lands too close to change detector tests.

I see your point.
I removed the check and added a comment.

codingllama · 2017-08-09T09:46:27Z

client/verifier_test.go

+func TestVerifyInclusionByHashErrors(t *testing.T) {
+	tests := []struct {
+		desc             string
+		trusted *trillian.SignedLogRoot


You're missing a gofmt run here. Try doing scripts/presubmit.sh --fix --no-generate, that should do all the formatting needed.

codingllama · 2017-08-09T09:47:07Z

client/verifier_test.go

+		if err == nil {
+			t.Errorf("%v: VerifyInclusionByHash() error expected, but none was thrown", test.desc)
+		}
+		if !strings.Contains(err.Error(), test.errorDesc){


nit: As above, I think it's fine to drop this.

gdbelvin · 2017-08-09T10:07:34Z

client/verifier_test.go

+		t.Fatalf("Failed to load public key, err=%v", err)
+	}
+
+	signedRoot := trillian.SignedLogRoot{0, nil, 0, nil, 0, 0}


this could be rewritten as trillian.SignedLogRoot{} since all the fields have their default values

deferring to codingllama

…ed by this PR)

AMarcedone · 2017-08-09T18:50:45Z

Updated. PTAL and thanks again :)

codingllama

Thanks, Antonio!

@gdbelvin, feel free to squash/merge after you approve.

codingllama · 2017-08-10T11:21:32Z

Oh, nevermind, I see that it was deferred to me. Merging.

AMarcedone added 2 commits August 4, 2017 20:35

Resolve nil pointer dereference bug and use getters for some of the p…

eb29481

…rotobuf fields.

Add Antonio Marcedone to AUTHORS and CONTRIBUTORS.

497889b

googlebot added the cla: yes label Aug 5, 2017

gdbelvin approved these changes Aug 7, 2017

View reviewed changes

gdbelvin reviewed Aug 7, 2017

View reviewed changes

gdbelvin previously requested changes Aug 7, 2017

View reviewed changes

codingllama suggested changes Aug 7, 2017

View reviewed changes

AMarcedone changed the title ~~Fix nil pointer dereference bug~~ Fix panic on logVerifier.VerifyRoot when newRoot is nil Aug 7, 2017

AMarcedone added 2 commits August 7, 2017 15:42

Add tests for client/logVerifier.VerifyRoot to prevent nil pointer de…

49a5ead

…reference errors/segfaults. Other minor nits in response to PR reviews.

fix import order

7eeab99

codingllama reviewed Aug 8, 2017

View reviewed changes

AMarcedone added 2 commits August 8, 2017 13:19

Reverted data_formats.go changes

62b0363

Move verifier tests to a separate file. Adress some review feedback.

acfd668

Make verification functions error early when given nil pointers as in…

03c7b51

…put.

AMarcedone force-pushed the fix_nil_pointer_dereference_bug branch from 29fdcb3 to 03c7b51 Compare August 8, 2017 22:48

codingllama reviewed Aug 9, 2017

View reviewed changes

gdbelvin reviewed Aug 9, 2017

View reviewed changes

AMarcedone added 2 commits August 9, 2017 11:36

scripts/presubmit.sh --fix --no-generate (only checked in files touch…

383c3f6

…ed by this PR)

Address some reviews

a312f4e

AMarcedone force-pushed the fix_nil_pointer_dereference_bug branch from b130d35 to a312f4e Compare August 9, 2017 18:37

Change inputs in test method to make purpose of the test more clear

0be2428

codingllama approved these changes Aug 10, 2017

View reviewed changes

codingllama merged commit 29bc71f into google:master Aug 10, 2017

Fix panic on logVerifier.VerifyRoot when newRoot is nil #779

Fix panic on logVerifier.VerifyRoot when newRoot is nil #779

Conversation

AMarcedone commented Aug 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AMarcedone Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdbelvin left a comment

Choose a reason for hiding this comment

codingllama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingllama Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingllama commented Aug 7, 2017

AMarcedone commented Aug 7, 2017

codingllama Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AMarcedone Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdbelvin commented Aug 8, 2017 via email

AMarcedone commented Aug 8, 2017

codingllama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AMarcedone commented Aug 9, 2017

codingllama left a comment

Choose a reason for hiding this comment

codingllama commented Aug 10, 2017

AMarcedone Aug 7, 2017 •

edited

Loading

codingllama Aug 8, 2017 •

edited

Loading

codingllama Aug 8, 2017 •

edited

Loading

AMarcedone Aug 8, 2017 •

edited

Loading