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

p2p/enode: store local port number as uint16 #23926

Merged
merged 1 commit into from Nov 23, 2021
Merged

p2p/enode: store local port number as uint16 #23926

merged 1 commit into from Nov 23, 2021

Conversation

jfcg
Copy link
Contributor

@jfcg jfcg commented Nov 17, 2021

Fix integer conversions discovered by CodeQL

@fjl
Copy link
Contributor

fjl commented Nov 17, 2021

We have decided against enabling CodeQL scanning in the past, for various reasons.

@fjl
Copy link
Contributor

fjl commented Nov 17, 2021

That said, would be interested to explain the reasoning behind the integer conversion change in package p2p/enode?

@jfcg
Copy link
Contributor Author

jfcg commented Nov 17, 2021

Port of an internet address has a well-defined range (hence type), and we need to utilize that when parsing a port string. For example "71000" should be treated equally invalid as "xyzw" when parsing a port field. This can yield to unpredictable problems, including security ones and CodeQL warns about these.

@fjl
Copy link
Contributor

fjl commented Nov 17, 2021

I don't think it is a security issue in this specific case because netutil.IPTracker will always return one of the strings added to it via AddStatement, and the statement strings are created by net.UDPAddr.String. However I do agree it is cleaner to use uint16.

Could you remove the CodeQL addition and leave only the integer type change in this PR? I would merge it then.

discovered by CodeQL
@jfcg jfcg changed the title Add Github CodeQL fix: integer conversions Nov 17, 2021
@fjl fjl changed the title fix: integer conversions p2p/enode: store local port number as uint16 Nov 17, 2021
@karalabe karalabe added this to the 1.10.13 milestone Nov 23, 2021
@fjl fjl removed the status:triage label Nov 23, 2021
@fjl fjl merged commit d15e423 into ethereum:master Nov 23, 2021
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Nov 23, 2021
yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
nibty pushed a commit to FairCrypto/go-ethereum that referenced this pull request Apr 10, 2024
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.

None yet

4 participants