Cast between semantically different integer types: HRESULT to/from bool #211

raulgarciamsft · 2018-09-20T23:18:56Z

Closing the gap between Semmle and PreFast.
Covers C6214, C6215, C6216, C6217, C6230

…Boolean type. Closing the gap between Semmle and PreFast. Covers C6214, C6215, C6216, C6217, C6230

jbj · 2018-09-21T09:52:17Z

The test failure is this:

01:39:16 [1019/1147] /mnt/resource/jenkins/workspace/Language-Tests/CPP/ql/cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.qlref: FAILED (compilation: N/A, execution: N/A, total: 1ms)
01:39:16 Could not find QL file Security/CWE/CWE-704/incorrectTypeConversion.ql

The file is named Security/CWE/CWE-704/IncorrectTypeConversion.ql, with an upper-case i.

jbj · 2018-09-21T12:13:01Z

cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.cpp

+	}
+
+	hr = S_FALSE;
+	if (hr) // Should fail


Should it say "BUG" here like for the other alerts?

jbj · 2018-09-21T12:21:49Z

cpp/ql/test/query-tests/Security/CWE/CWE-704/IncorrectTypeConversion.cpp

+	BOOL b = IncorrectHresultFunction(); // BUG
+
+	hr = E_UNEXPECTED;
+	if (!hr) // BUG


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.

I will work on it right away.

jbj · 2018-09-21T14:11:17Z

cpp/ql/src/Security/CWE/CWE-704/IncorrectTypeConversion.ql

+ * @problem.severity error
+ * @precision high
+ * @tags security
+ *       external/cwe/cwe-704


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.

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.

jbj · 2018-09-21T14:13:36Z

cpp/ql/src/Security/CWE/CWE-704/IncorrectTypeConversion.ql

+ *              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


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.

Sounds good. How about "cpp/hresult-to-boolean-conversion" (please feel free to suggest another one if you prefer).

raulgarciamsft · 2018-09-21T18:11:33Z

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

rdmarsh2 · 2018-09-21T18:19:34Z

Consider CWE-253: Incorrect Check of Function Return Value

jbj · 2018-09-21T18:33:52Z

Thanks, @rdmarsh2. CWE-253 looks right for this query.

I like cpp/hresult-to-boolean-conversion. You can also remove the -to since it goes both ways. For the file name(s), I suggest renaming to HResultBooleanConversion.*.

jbj · 2018-09-21T19:46:27Z

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.

raulgarciamsft · 2018-09-21T20:01:07Z

I am changing the CWE information to CWE-253.
I will also search for the tabs. I thought I replaced them, but it seems like I missed some. Thanks

jbj · 2018-09-24T06:50:45Z

cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.qhelp

+
+<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" />


This reference to IncorrectTypeConversion.cpp needs to change to HResultBooleanConversion.cpp. When that's done, I'm happy to merge this PR.

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

raulgarciamsft and others added 2 commits September 20, 2018 16:16

Cast between semantically different integer types: HRESULT to/from a …

b0ec929

…Boolean type. Closing the gap between Semmle and PreFast. Covers C6214, C6215, C6216, C6217, C6230

Merge branch 'master' into users/raulga/HESULT

64b8a5b

jbj added the C++ label Sep 21, 2018

jbj reviewed Sep 21, 2018

View reviewed changes

raulgarciamsft added 2 commits September 21, 2018 11:14

Fixing typos & ID.

783be15

NOTE: There is an ongoing discussion on the proper CWE we should use

Merge branch 'users/raulga/HESULT' of https://github.com/raulgarciams…

94f752c

…ft/ql into users/raulga/HESULT

Major change in order to support the rule for C as well as cpp

242ee10

jbj reviewed Sep 24, 2018

View reviewed changes

Fixing a missed reference to renamed file.

c3b523c

jbj approved these changes Sep 25, 2018

View reviewed changes

jbj merged commit 8f19efe into github:master Sep 25, 2018

jbj mentioned this pull request Sep 26, 2018

C++: Finalise docs for cpp/hresult-boolean-conversion and cpp/unsafe-dacl-security-descriptor #235

Merged

jbj mentioned this pull request Dec 17, 2018

cpp - Using the return value of a strcpy or related string copy function in an if statement #695

Merged

raulgarciamsft deleted the users/raulga/HESULT branch October 4, 2019 16:58

aibaars pushed a commit that referenced this pull request Oct 14, 2021

Merge pull request #211 from github/smaller_trap

78db1bf

Tweaks to reduce size of TRAP output

smowton pushed a commit to smowton/codeql that referenced this pull request Feb 7, 2022

Merge pull request github#211 from github/igfoo/propTest

a7ced8b

Kotlin: Add a test for Kotlin seeing Java code as properties

MathiasVP pushed a commit to MathiasVP/ql that referenced this pull request Aug 10, 2025

Merge pull request github#211 from microsoft/dilan/publish-pack-bug-2

2f62351

Pack Publish Bug

Cast between semantically different integer types: HRESULT to/from bool #211

Cast between semantically different integer types: HRESULT to/from bool #211

Uh oh!

Conversation

raulgarciamsft commented Sep 20, 2018

Uh oh!

jbj commented Sep 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raulgarciamsft commented Sep 21, 2018

Uh oh!

rdmarsh2 commented Sep 21, 2018

Uh oh!

jbj commented Sep 21, 2018

Uh oh!

jbj commented Sep 21, 2018

Uh oh!

raulgarciamsft commented Sep 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants