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

avoid data copy and buffer allocation when read out p2p message from network #566

Merged
merged 2 commits into from
Aug 28, 2018

Conversation

laizy
Copy link
Contributor

@laizy laizy commented Jul 26, 2018

Signed-off-by: laizy laizhichao@onchain.com

lightshine001
lightshine001 previously approved these changes Jul 26, 2018
Copy link
Contributor

@lightshine001 lightshine001 left a comment

Choose a reason for hiding this comment

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

It’s ok for me. Please confirm ut passed.

Signed-off-by: laizy <laizhichao@onchain.com>
"errors"
"fmt"
"net"
"time"

common2 "github.com/ontio/ontology/common"

Choose a reason for hiding this comment

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

use another name rather than "pkg+2"??

this.NodeAddrs = append(this.NodeAddrs, addr)
}

if count > comm.MAX_ADDR_NODE_CNT {

Choose a reason for hiding this comment

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

count exceed attack check should before node address appending

Copy link
Contributor Author

@laizy laizy Aug 27, 2018

Choose a reason for hiding this comment

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

  1. the param is not []byte any more, so all bytes belong to this message should be read out from the source, to avoid subtle bug. 2. the max payload size has checked, so this is safe.

Choose a reason for hiding this comment

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

so this is safe now, should we remove this count limit??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since there is a global max outbound connection limit, the MAX_ADDR_NODE_CNT is to avoid malicious node send too many addresses and occupy the outbound connection?

}

//Deserialize message payload
func (this *Addr) Deserialize(r io.Reader) error {

Choose a reason for hiding this comment

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

remove Deserialize ??

this.P.Blk = append(this.P.Blk, hash)
}

if blkCnt > p2pCommon.MAX_INV_BLK_CNT {

Choose a reason for hiding this comment

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

check before blk append??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same reason as above

@laizy laizy merged commit 4f184b9 into ontio:master Aug 28, 2018
@laizy laizy deleted the p2p_buffer branch August 28, 2018 10:39
lightshine001 pushed a commit to lightshine001/ontology that referenced this pull request Sep 14, 2018
…network (ontio#566)


Signed-off-by: laizy <laizhichao@onchain.com>
lightshine001 pushed a commit to lightshine001/ontology that referenced this pull request Sep 14, 2018
…network (ontio#566)


Signed-off-by: laizy <laizhichao@onchain.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants