CodeQL ilasm fixes by dhartglassMSFT · Pull Request #125152 · dotnet/runtime

dhartglassMSFT · 2026-03-03T23:10:44Z

Fix two issues that got flagged

1 - Fix a mistyped loop counter.

2 - One of the files frequently casts CHAR*->WCHAR*. Retyping isn't reasonable, because these methods are called through function pointers using char*. These casts are probably safe since they all point into MapViewOfFile, but I add alignment constraints and bailouts if somehow we have an odd pointer. The analyzer still complains however, so I also need an explicit suppression of the warning.

I'm kicking off another analysis pipeline in a bit to make sure the suppressions get picked up

dotnet-policy-service · 2026-03-03T23:11:52Z

Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib
See info in area-owners.md if you want to be subscribed.

Copilot

Pull request overview

This PR targets CodeQL/static-analysis findings in the CoreCLR ilasm tool by addressing integer type correctness and potential unaligned UTF-16 access patterns in the parser/helpers.

Changes:

Use an unsigned 16-bit loop index when iterating metadata streams during PE file creation.
Avoid unaligned WCHAR* dereferences in SymW by reading via memcpy.
Add/adjust alignment handling for Unicode parsing helpers (aligned buffers; enforce even-byte lengths for UTF-16 token copies).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
`src/coreclr/ilasm/writer.cpp`	Updates stream-iteration index type to avoid signed/size issues when walking metadata streams.
`src/coreclr/ilasm/grammar_after.cpp`	Hardens Unicode parsing helpers against unaligned access and odd-length UTF-16 token handling (plus aligned scratch buffers).

src/coreclr/ilasm/grammar_after.cpp

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

src/coreclr/ilasm/writer.cpp

src/coreclr/ilasm/grammar_after.cpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Copilot · 2026-03-04T22:33:46Z

src/coreclr/ilasm/grammar_after.cpp

            uCount &= 0x1FFF; // limit: 8K wchars
+            // CodeQL [SM02986] this cast is safe because buff is WCHAR aligned and we null terminate below
            WCHAR* wzBuff = (WCHAR*)buff;
            memcpy(buff,pLine,uCount);
            wzBuff[uCount >> 1] = 0;


In the Unicode path, buff is a char[] that is cast to WCHAR* and written through (wzBuff[uCount >> 1] = 0;). alignas(WCHAR) fixes alignment but not the strict-aliasing/effective-type issue of writing a WCHAR into storage declared as char[]. Consider using a WCHAR-typed buffer for the Unicode path (or a union/std::aligned_storage approach) so the terminator write is well-defined without relying on a suppression.

src/coreclr/ilasm/grammar_after.cpp

Copilot · 2026-03-04T22:33:46Z

src/coreclr/ilasm/grammar_after.cpp

+    alignas(WCHAR) static char dbuff[256];
    char* pdummy = NULL;
    if(L > 254) L = 254;
+    // L must be even in the Unicode case, so the null
+    // termination isn't split across two WCHARs
+    L &= ~(size_t)1;
    memcpy(dbuff,begNum,L);
    dbuff[L] = 0;
    dbuff[L+1] = 0;
+    // CodeQL [SM02986] Cast to WCHAR is safe because dbuff is properly aligned and we ensure proper null termination above
    *ppRes = new double(u16_strtod((const WCHAR*)dbuff, (WCHAR**)&pdummy));
    return ((unsigned)(pdummy - dbuff));


In GetDoubleW, dbuff is a char[] that is later reinterpreted as const WCHAR* for u16_strtod. Even with alignas(WCHAR), accessing a char[] through a WCHAR* can violate C++ strict-aliasing/effective-type rules and is vulnerable to miscompilation under optimization. Consider changing the temporary buffer to a WCHAR[] (and tracking L/pdummy in WCHAR units) to avoid the cast and the CodeQL suppression entirely.

Suggested change

alignas(WCHAR) static char dbuff[256];

char* pdummy = NULL;

if(L > 254) L = 254;

// L must be even in the Unicode case, so the null

// termination isn't split across two WCHARs

L &= ~(size_t)1;

memcpy(dbuff,begNum,L);

dbuff[L] = 0;

dbuff[L+1] = 0;

// CodeQL [SM02986] Cast to WCHAR is safe because dbuff is properly aligned and we ensure proper null termination above

*ppRes = new double(u16_strtod((const WCHAR*)dbuff, (WCHAR**)&pdummy));

return ((unsigned)(pdummy - dbuff));

static WCHAR dbuff[128];

WCHAR* pdummy = NULL;

// Limit L (in bytes) to leave room for a terminating WCHAR null.

const unsigned maxBytes = static_cast<unsigned>(sizeof(dbuff) - sizeof(WCHAR));

if (L > maxBytes)

{

L = maxBytes;

}

// L must be even in the Unicode case, so the null

// termination isn't split across two WCHARs.

L &= ~(size_t)1;

memcpy(dbuff, begNum, L);

const size_t wcharCount = L / sizeof(WCHAR);

dbuff[wcharCount] = 0;

*ppRes = new double(u16_strtod(dbuff, &pdummy));

const size_t consumedWChars = static_cast<size_t>(pdummy - dbuff);

return static_cast<unsigned>(consumedWChars * sizeof(WCHAR));

I believe we compile with fno strict aliasing

src/coreclr/ilasm/grammar_after.cpp

src/coreclr/ilasm/writer.cpp

src/coreclr/ilasm/grammar_after.cpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

am11 · 2026-03-05T19:47:07Z

I think focus should move to the managed ilasm in src/tools/ilasm, which will eventually make the native implementation obsolete.

AaronRobinsonMSFT · 2026-03-05T20:59:03Z

I think focus should move to the managed ilasm in src/tools/ilasm, which will eventually make the native implementation obsolete.

Until that is working, which is a long ways off, we still need to satisfy tooling and all the requirements for the existing code.

am11 · 2026-03-05T21:09:01Z

which is a long ways off,

It is just pending test infra support, everything else is in place; all TODOs addressed.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

src/coreclr/ilasm/grammar_after.cpp

AaronRobinsonMSFT

src/coreclr/ilasm/grammar_after.cpp

Co-authored-by: Aaron R Robinson <arobins@microsoft.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

src/coreclr/ilasm/grammar_after.cpp

src/coreclr/ilasm/asmparse.h

dhartglassMSFT added 2 commits March 1, 2026 16:14

iStr change short to USHORT so type on compare matches

517c7fc

safety checks for char to wchar string conversions

05adcc7

Copilot AI review requested due to automatic review settings March 3, 2026 23:10

github-actions bot added the area-ILTools-coreclr label Mar 3, 2026

dotnet-policy-service bot assigned dhartglassMSFT Mar 3, 2026

Copilot started reviewing on behalf of dhartglassMSFT March 3, 2026 23:12 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

src/coreclr/ilasm/grammar_after.cpp Show resolved Hide resolved

src/coreclr/ilasm/grammar_after.cpp Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Mar 4, 2026

[android] Android.Device_Emulator.JIT.Test failing on emulators with CoreCLR #112633

Open

add explicit codeql suppression

902a4db

dhartglassMSFT requested a review from AaronRobinsonMSFT March 4, 2026 21:44

dhartglassMSFT marked this pull request as ready for review March 4, 2026 22:21

Copilot AI review requested due to automatic review settings March 4, 2026 22:21

Copilot started reviewing on behalf of dhartglassMSFT March 4, 2026 22:22 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

src/coreclr/ilasm/writer.cpp Outdated Show resolved Hide resolved

src/coreclr/ilasm/grammar_after.cpp Outdated Show resolved Hide resolved

dhartglassMSFT changed the title ~~[Draft] CodeQL ilasm fixes~~ CodeQL ilasm fixes Mar 4, 2026

Update src/coreclr/ilasm/writer.cpp

ad6b3b4

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings March 4, 2026 22:29

Copilot started reviewing on behalf of dhartglassMSFT March 4, 2026 22:30 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

AaronRobinsonMSFT reviewed Mar 4, 2026

View reviewed changes

src/coreclr/ilasm/grammar_after.cpp Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Mar 4, 2026

View reviewed changes

src/coreclr/ilasm/writer.cpp Show resolved Hide resolved

AaronRobinsonMSFT reviewed Mar 4, 2026

View reviewed changes

src/coreclr/ilasm/grammar_after.cpp Outdated Show resolved Hide resolved

Update src/coreclr/ilasm/grammar_after.cpp

c2c2c9e

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings March 5, 2026 00:53

Copilot started reviewing on behalf of dhartglassMSFT March 5, 2026 00:54 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

use local wchar buffer instead in GetDoubleW

8020ce7

build-analysis bot mentioned this pull request Mar 5, 2026

Android WebSocket failure #121518

Open

pr feedback

b628079

Copilot AI review requested due to automatic review settings March 9, 2026 20:26

Copilot started reviewing on behalf of dhartglassMSFT March 9, 2026 20:27 View session

Copilot AI reviewed Mar 9, 2026

View reviewed changes

src/coreclr/ilasm/grammar_after.cpp Show resolved Hide resolved

src/coreclr/ilasm/grammar_after.cpp Outdated Show resolved Hide resolved

update comment

cc89ff6

This was referenced Mar 10, 2026

iOS tests failing with WORKLOAD TIMED OUT - Killing user command. #108103

Open

XHarness package install failure on iOS due to devicectl NSPOSIXErrorDomain error 49 #123796

Open

[8.0] System.Exception: Failed to list devices - tvOS, iOS #125135

Open

AaronRobinsonMSFT approved these changes Mar 10, 2026

View reviewed changes

src/coreclr/ilasm/grammar_after.cpp Outdated Show resolved Hide resolved

declare PtrIsWCHARAligned as static

fc576f9

Co-authored-by: Aaron R Robinson <arobins@microsoft.com>

Copilot AI review requested due to automatic review settings March 10, 2026 21:00

Copilot started reviewing on behalf of dhartglassMSFT March 10, 2026 21:02 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

src/coreclr/ilasm/grammar_after.cpp Show resolved Hide resolved

src/coreclr/ilasm/grammar_after.cpp Show resolved Hide resolved

src/coreclr/ilasm/asmparse.h Show resolved Hide resolved

dhartglassMSFT enabled auto-merge (squash) March 10, 2026 21:18

Conversation

dhartglassMSFT commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Mar 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

dhartglassMSFT Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

am11 commented Mar 5, 2026

Uh oh!

AaronRobinsonMSFT commented Mar 5, 2026

Uh oh!

am11 commented Mar 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dhartglassMSFT commented Mar 3, 2026 •

edited

Loading