Fix a number of CodeQL warnings #14882
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dbussink
added
Type: Internal Cleanup
Component: General
dbussink
requested review from
deepthi,
harshit-gangal,
mattlord,
ajm188,
notfelineit,
shlomi-noach,
GuptaManan100,
rohit-nayak-ps and
frouioui
as code owners
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
dbussink
commented
Jan 3, 2024
| uses: actions/setup-go@v4 | ||
| with: | ||
| go-version: 1.21.5 | ||
|
|
There was a problem hiding this comment.
dbussink
removed
NeedsDescriptionUpdate
dbussink
force-pushed
the
dbussink/fix-codeql-warnings
branch
from
dee3d90 to
4f77d60
Compare
This addresses a number of CodeQL warnings. We have a concurrency field which is using inconsistent types across different fields, which means we end up casting and triggering warnings around overflows. We can use the smallest type here, namely int32 since that is for sure sufficient for real concurrency values. It avoids warnings like: https://github.com/vitessio/vitess/security/code-scanning/267 Also fixes the parsing bug identified in https://github.com/vitessio/vitess/security/code-scanning/266 where we parse with `ParseUint` instead of `ParseInt`. Last one is https://github.com/vitessio/vitess/security/code-scanning/268 which is fixed by making sure we parse it as the type MySQL sends down and to keep using that consistently. Signed-off-by: Dirkjan Bussink <[email protected]>
dbussink
force-pushed
the
dbussink/fix-codeql-warnings
branch
from
4f77d60 to
560496b
Compare
shlomi-noach
approved these changes
Jan 4, 2024
| // name into a uint32 value. If the parameter is not set, the provided default | ||
| // value is returned. | ||
| func (r Request) ParseQueryParamAsInt32(name string, defaultVal int32) (int32, error) { | ||
| if param := r.URL.Query().Get(name); param != "" { |
There was a problem hiding this comment.
Styling-wise, I'd prefer to first handle the case where param == "" and do an early return, then unindent the rest of the function. But it's insignificant and not worth the rebuild time.
There was a problem hiding this comment.
@shlomi-noach Copy-pasted this mostly from another function for uint32, so it's not "new" in that sense.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses a number of CodeQL warnings. We have a concurrency field which is using inconsistent types across different fields, which means we end up casting and triggering warnings around overflows. We can use the smallest type here, namely int32 since that is for sure sufficient for real concurrency values. It avoids warnings like:
https://github.com/vitessio/vitess/security/code-scanning/267
Also fixes the parsing bug identified in
https://github.com/vitessio/vitess/security/code-scanning/266 where we parse with
ParseUintinstead ofParseInt.Last one is
https://github.com/vitessio/vitess/security/code-scanning/268 which is fixed by making sure we parse it as the type MySQL sends down and to keep using that consistently.
Checklist