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

Jump to bottom

C#: Explicitly quote arguments in the LUA tracer on windows. #14150

Merged
merged 2 commits into from Sep 11, 2023
Merged

C#: Explicitly quote arguments in the LUA tracer on windows. #14150

merged 2 commits into from Sep 11, 2023
+4 −9

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Sep 6, 2023 •
edited

If quoted arguments are used these will not be respected by the LUA tracer on windows.
In this PR we explicitly quote all compiler arguments containing whitespaces (windows only) in the dotnet matcher.

Something similar is already being done in the dotnet exec matcher: https://github.com/github/codeql/blob/main/csharp/tools/tracing-config.lua#L177

@github-actions github-actions bot added the C# label Sep 6, 2023
@michaelnebel
Copy link
Contributor Author

From the tracer log we can see that

==== Candidate to intercept: C:\Program Files\dotnet\dotnet.exe (2840) ====
[T 09:05:07 8112] Lua: Dotnet subcommand detected: run
[T 09:05:07 8112] Executing the following tracer actions:
[T 09:05:07 8112] Tracer actions:
[T 09:05:07 8112] pre_invocations(0)
[T 09:05:07 8112] replacing compiler with:
[T 09:05:07 8112] invocation: c:\program files\dotnet\dotnet.exe, args: run -p:UseSharedCompilation=false -p:EmitCompilerGeneratedFiles=true -- hello world arg2

which explains the failure: "hello world" is missing quotes.

@michaelnebel michaelnebel force-pushed the csharp/tracerwhitespace branch 4 times, most recently from 29350bc to 7b9a6ea Compare September 6, 2023 12:31
@michaelnebel michaelnebel changed the title C#: Integration tests using whitespaces in dotnet commands. C#: Explicitly quote arguments in the LUA tracer on windows. Sep 6, 2023
@michaelnebel michaelnebel added the no-change-note-required This PR does not need a change note label Sep 6, 2023
@DkSkydancer
Copy link

@michaelnebel
Please make sure the command dotnet test --collect "Code Coverage" is is supported!

aibaars
aibaars reviewed Sep 7, 2023
View reviewed changes
csharp/tools/tracing-config.lua Outdated Show resolved Hide resolved
csharp/tools/tracing-config.lua Outdated Show resolved Hide resolved
@michaelnebel michaelnebel added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Sep 8, 2023
aibaars
aibaars previously approved these changes Sep 8, 2023
View reviewed changes
@michaelnebel michaelnebel mentioned this pull request Sep 11, 2023
Merged
@michaelnebel michaelnebel marked this pull request as ready for review September 11, 2023 09:55
@michaelnebel michaelnebel requested a review from a team as a code owner September 11, 2023 09:55
@michaelnebel michaelnebel merged commit aaaf6f8 into github:main Sep 11, 2023
14 checks passed
@michaelnebel michaelnebel deleted the csharp/tracerwhitespace branch September 11, 2023 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbartol dbartol dbartol left review comments

@tamasvajk tamasvajk tamasvajk approved these changes

@aibaars aibaars aibaars left review comments

Assignees
No one assigned
Labels
C# depends on internal PR This PR should only be merged in sync with an internal Semmle PR no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@michaelnebel @DkSkydancer @aibaars @tamasvajk @dbartol