-
Notifications
You must be signed in to change notification settings - Fork 1.7k
C++: Add Windows command line and environment models #19563
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds Windows command-line and environment models to the C++ dataflow tests and core library, enabling flow-tracking for WinMain parameters and Win32 API calls.
- Introduce
windows.cpp
andwinmain.cpp
tests coveringGetCommandLineA/W
,CommandLineToArgvA/W
,GetEnvironmentStringsA/W
,GetEnvironmentVariableA/W
, and WinMain’spCmdLine
. - Update expected
.expected
files to include new source, step, flow, and irFlow entries for those tests. - Add a
CmdLineSource
inFlowSources.qll
, import it inTestBase.qll
, and define YML models inWindows.model.yml
; document the change inchange-notes
.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
cpp/ql/test/library-tests/dataflow/external-models/windows.cpp | New tests exercising Windows command-line & env APIs |
cpp/ql/test/library-tests/dataflow/dataflow-tests/winmain.cpp | New WinMain test for pCmdLine source |
cpp/ql/test/library-tests/dataflow/external-models/steps.expected | Updated step expectations for CommandLineToArgvA |
cpp/ql/test/library-tests/dataflow/external-models/sources.expected | Added sources for Win32 APIs |
cpp/ql/test/library-tests/dataflow/external-models/flow.expected | Added flow edges for Windows external models |
cpp/ql/test/library-tests/dataflow/dataflow-tests/test-source-sink.expected | Added WinMain irFlow entry |
cpp/ql/test/library-tests/dataflow/dataflow-tests/TestBase.qll | Imported new FlowSources and extended isSource |
cpp/ql/lib/semmle/code/cpp/security/FlowSources.qll | Added CmdLineSource model for WinMain’s pCmdLine |
cpp/ql/lib/ext/Windows.model.yml | Defined summary/source models for Win32 APIs |
cpp/ql/lib/change-notes/2025-05-23-windows-sources.md | Documented the new Windows sources and models |
Comments suppressed due to low confidence (1)
cpp/ql/test/library-tests/dataflow/external-models/steps.expected:8
- The table entry refers to
*cmd
at column 16:36–16:38, but the dereference in the code is*argv[1]
. This should be updated to reference*argv[1]
to match the actual code under test.
| windows.cpp:16:36:16:38 | *cmd | windows.cpp:16:17:16:34 | **call to CommandLineToArgvA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@@ -0,0 +1,20 @@ | |||
# partial model of windows system calls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# partial model of windows system calls | |
# partial model of windows system calls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledging I saw this. Will fix this in a separate PR.
int GetEnvironmentVariableA(const char*, char*, int); | ||
|
||
void getCommandLine() { | ||
char* cmd = GetCommandLineA(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe these A
and W
variants are often called via a macro with no letter (in this case it would be GetCommandLine
). It's probably fine that we test on the specific implementation functions and I suspect many users do in fact call them directly as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
New results in DCA look genuine.