Skip to content

add handshakeTimeout, bound chunkLength, comments#113

Merged
jaekwon merged 2 commits intodevelopfrom
sconn
Jul 19, 2015
Merged

add handshakeTimeout, bound chunkLength, comments#113
jaekwon merged 2 commits intodevelopfrom
sconn

Conversation

@ebuchman
Copy link
Contributor

  • ReadFull hangs forever unless we set a timeout on the conn (not sure this matters for sconn.Read(), but it does for shareEphPubKey
  • sconn.Read was missing a bounds check such that a malicious chunkLength would crash the node
  • there's also a question in there about the size of the buffer used for shareAuthSignature

Unfortunately, I couldn't think of an easy way to implement tests for my fixes (I tested both manually).

@ebuchman
Copy link
Contributor Author

crypto byte arrays (pubkey, priv key, sig for ed25519) are all fixed length now (so we can call ReadFull on exactly the right sized read buffer in shareAuthSigs)
I added a very basic test (ie. a "it doesnt panic" test) for fixed length arrays, will still add more.
NOTE: moving to fixed length breaks the previous pubkey->addr map because pubkey.Address() binary encodes the pubkey before hashing

Copy link
Contributor

Choose a reason for hiding this comment

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

was this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't like it. But with the old code it seemed to be hex encoding the call to eg pubKey.String()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you have a better way to do this?

@jaekwon jaekwon merged commit 7865cd3 into develop Jul 19, 2015
@ebuchman ebuchman deleted the sconn branch July 19, 2015 22:51
ebuchman added a commit that referenced this pull request Jun 20, 2018
Fix #112 by using RWMutex per element
liamsi pushed a commit to liamsi/tendermint that referenced this pull request Jun 21, 2018
faddat pushed a commit to faddat/tendermint that referenced this pull request Nov 23, 2022
BadgerDB is pretty heavy, let's not include it by default. Related to tendermint#113.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants