treewide: Fix problems identified by CodeQL #17516
Conversation
twpayne
added
the
release-note/misc
|
/test |
proxylib/testparsers/blockparser.go
Outdated
| } else if lenUint64 > math.MaxInt64 { | ||
| return block.Bytes(), 0, 0, fmt.Errorf("block length overflow") |
There was a problem hiding this comment.
Couple of questions here:
- Why compare a uint64 type against an int64 size? Is there code lower down that is limiting this to the positive 63-bit range or signed integer comparisons below?
- Why is this a problem if
ParseUint(..., 64)is limiting it to 64 bits? I understand that the result is auintwhich doesn't have a defined size, but presumably the last parameter toParseUint()should bound that within 64 bits.
The more I look at this particular change, the more I wonder if the complaint is less about the 64-bit size up here but more to do with some comparison against a shorter type below 🤔
There was a problem hiding this comment.
Why compare a uint64 type against an int64 size?
We parse a value using strconv.ParseUint (which returns a uint64) but we store the value in blockLen (which is an int), so any value greater that math.MaxInt64 will overflow the int (assuming a 64 bit platform).
The more I look at this particular change, the more I wonder if the complaint is less about the 64-bit size up here but more to do with some comparison against a shorter type below 🤔
It's possible. We only run CodeQL against master and it can be difficult to trace exactly why CodeQL complains, so it's possible that this change won't fix CodeQL's complaints.
There was a problem hiding this comment.
Ah right. If two lines down we're casting down to int(...) then seems like we should be doing a math.MaxInt() here instead?
There was a problem hiding this comment.
Done. Note that math.MaxInt isn't available at the moment as our runtime CI VMs are still running Go 1.16, but I've left a FIXME to switch this to math.MaxInt in the code for when we get them upgraded.
In the short term, I've used the max int64 constant, which is not correct if Cilium is compiled for a 32-bit architecture, but switching to math.MaxInt later will fix this.
|
/test |
|
test-runtime failed because the CI runtime VMs are still running Go 1.16 (waiting for #17394). I've added a workaround. |
|
/ci-multicluster |
|
/test-gke |
|
/test-runtime |
|
/test |
|
/test |
CodeQL identified "Incorrect conversion of a 64-bit integer from to a lower bit size type uint16 without an upper bound check". Signed-off-by: Tom Payne <tom@isovalent.com>
CodeQL identified multiple "Incorrect conversion of a 64-bit integer from to a lower bit size type uint32 without an upper bound check.". Signed-off-by: Tom Payne <tom@isovalent.com>
CodeQL identified "Incorrect conversion of a 64-bit integer from to a lower bit size type int without an upper bound check." Signed-off-by: Tom Payne <tom@isovalent.com>
CodeQL identified "Incorrect conversion of a 64-bit integer from to a lower bit size type int without an upper bound check." Signed-off-by: Tom Payne <tom@isovalent.com>
CodeQL identified "This regular expression has an unescaped dot before 'com', so it might match more hosts than expected when used ." Signed-off-by: Tom Payne <tom@isovalent.com>
|
/test |
2 similar comments
|
/test |
|
/test |
|
CI is mostly green, marking as |
twpayne
added
the
ready-to-merge
This PR fixes a number of problems identified by CodeQL.
See individual commits for details.