Cast between semantically different integer types: HRESULT to/from bool#211
Conversation
…Boolean type. Closing the gap between Semmle and PreFast. Covers C6214, C6215, C6216, C6217, C6230
|
The test failure is this: The file is named |
| } | ||
|
|
||
| hr = S_FALSE; | ||
| if (hr) // Should fail |
There was a problem hiding this comment.
Should it say "BUG" here like for the other alerts?
| BOOL b = IncorrectHresultFunction(); // BUG | ||
|
|
||
| hr = E_UNEXPECTED; | ||
| if (!hr) // BUG |
There was a problem hiding this comment.
This test file has extension .cpp, so !hr includes a conversion to bool. Please add a copy of this test file with the .c extension so we can verify what happens there. The C language has only had a bool type since C99, and even there it's usually a typedef for a built-in type _Bool, so I'd like to be sure that the query works in a C file as well.
To be sure that the C test, and this one, applies to the compiler that most people will be using, please add // semmle-extractor-options: --microsoft anywhere in the file.
There was a problem hiding this comment.
I will work on it right away.
| * @problem.severity error | ||
| * @precision high | ||
| * @tags security | ||
| * external/cwe/cwe-704 |
There was a problem hiding this comment.
I'm wondering if CWE-704 is right for this query. I interpret that CWE to be about invalid casts that cause undefined behaviour rather than the logic errors that this query is looking for.
There was a problem hiding this comment.
Any recommendation on a better fitting CWE?
I also considered CWE-681 (Incorrect Conversion between Numeric Types), but it doesn't really fit very well either.
| * Boolean types indicate success by a non-zero value, whereas success (S_OK) in HRESULT is indicated by a value of 0. | ||
| * Casting an HRESULT to/from a Boolean type and then using it in a test expression will yield an incorrect result. | ||
| * @kind problem | ||
| * @id cpp/incorrect-type-conversion |
There was a problem hiding this comment.
I think this ID and the file name of the query are too generic. It's a very specific query about conversions between HRESULT and bool, and the names should reflect that.
There was a problem hiding this comment.
Sounds good. How about "cpp/hresult-to-boolean-conversion" (please feel free to suggest another one if you prefer).
|
There is an outstanding conversation regarding the proper CWE to use. I will submit the fixes I have ready, but I am expecting that the follow up on the CWE discussion will mean more changes. |
NOTE: There is an ongoing discussion on the proper CWE we should use
…ft/ql into users/raulga/HESULT
|
Thanks, @rdmarsh2. CWE-253 looks right for this query. I like |
|
I just noticed that some files use a mix of tabs and spaces. Sorry, but we have a CI check for that that only runs internally and not on external PRs. It'll start failing if we merge this. Please change tabs to spaces in all files. |
|
I am changing the CWE information to CWE-253. |
|
|
||
| <example> | ||
| <p>In the following example, <code>HRESULT</code> is used in a test expression incorrectly as it may yield an incorrect result.</p> | ||
| <sample src="IncorrectTypeConversion.cpp" /> |
There was a problem hiding this comment.
This reference to IncorrectTypeConversion.cpp needs to change to HResultBooleanConversion.cpp. When that's done, I'm happy to merge this PR.
There was a problem hiding this comment.
Fixed. Thanks a lot
Tweaks to reduce size of TRAP output
Kotlin: Add a test for Kotlin seeing Java code as properties
Pack Publish Bug
Closing the gap between Semmle and PreFast.
Covers C6214, C6215, C6216, C6217, C6230