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

Fixed warnings for Implicit narrowing conversion in compound assignment #1758

Merged
merged 5 commits into from
Mar 3, 2022

Conversation

Jeffery-Wasty
Copy link
Member

@Jeffery-Wasty Jeffery-Wasty commented Mar 1, 2022

(Update 3/2/2022) While the below approach works, given that the use of readSkipBytes has changed, its no longer necessary to pass in a long. Thus the changes can be reduced to ensuring all involved types are int, avoiding any information loss.


As seen here: https://github.com/microsoft/mssql-jdbc/security/code-scanning/1?query=ref%3Arefs%2Fheads%2Fmain, there is an implicit cast taking place in IOBuffer.java, that may lead to a loss of information. The solution was to first check if the long value bytesToSkip can be converted without information loss, and only allow addition to payloadOffset if this is true.

The reverse, that is widening payloadOffset to long, to accommodate bytesToSkip is not possible because of the many uses payloadOffset has throughout the file. Lines 7043 and 7049 in IOBuffer.java are good examples of this. Extensive reformatting is needed to change payloadOffset, while bytesToSkip can be checked with ease.

The function used to check if bytesToSkip is small enough, isSafeToConvert, uses a switch function even though it only has one case, this is to allow future cases to be added, if needed. Additionally, other values, besides long, can be passed in to convert, provided they are cast beforehand.

@tkyc
Copy link
Member

tkyc commented Mar 2, 2022

I might be missing something here, but isn't an alternative like changing the "valueLength" param to int and then keeping the rest of the types in the method to int possible also? I only see the "readSkipBytes" method used in one place and it is taking in an int only, never a long

@Jeffery-Wasty
Copy link
Member Author

You know what, I think you're absolutely right. I'm looking at the initial implementation, and it may have been necessary to pass in a long in that case, but that's no longer needed. This simplifies things greatly.

@Jeffery-Wasty Jeffery-Wasty added this to Under Investigation in MSSQL JDBC via automation Mar 2, 2022
@Jeffery-Wasty Jeffery-Wasty moved this from Under Investigation to Under Peer Review in MSSQL JDBC Mar 2, 2022
@lilgreenbird lilgreenbird added this to the 10.3.0 milestone Mar 3, 2022
@Jeffery-Wasty Jeffery-Wasty merged commit a16a2a7 into main Mar 3, 2022
MSSQL JDBC automation moved this from Under Peer Review to Closed/Merged PRs Mar 3, 2022
@Jeffery-Wasty Jeffery-Wasty deleted the Implicit-narrowing-conversion branch March 3, 2022 21:34
@lilgreenbird lilgreenbird changed the title Added a check to prevent Implicit narrowing conversion in compound assignment Fixed warnings for Implicit narrowing conversion in compound assignment Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MSSQL JDBC
  
Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

None yet

4 participants