Skip to content

Fix Sve test templates to avoid buffer overrun #113113

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.

Sign up for GitHub

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
Merged
merged 3 commits into from
Mar 5, 2025
Merged

Fix Sve test templates to avoid buffer overrun #113113

merged 3 commits into from
Mar 5, 2025

Conversation

kunalspathak
Copy link
Contributor

Fixes: #112463

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 4, 2025
@kunalspathak
Copy link
Contributor Author

/azp run runtime-coreclr gcstress-extra

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Mar 4, 2025
Closed
Open
@kunalspathak
Copy link
Contributor Author

/azp run runtime-coreclr gcstress-extra

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kunalspathak
Copy link
Contributor Author

windows-arm64 leg is passing. There is currently linux infra issue because of which other linux are failing. I will wait for it to resolve before scheduling another run, but the changes in this PR should fix the original issue.

@kunalspathak kunalspathak marked this pull request as ready for review March 4, 2025 22:20
@Copilot Copilot AI review requested due to automatic review settings March 4, 2025 22:20
Copilot
Copilot AI reviewed Mar 4, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kunalspathak
Copy link
Contributor Author

@dotnet/arm64-contrib

@kunalspathak kunalspathak changed the title Fix SveLoadVectorFirstFaultingTest.template Fix Sve test templates to avoid buffer overrun Mar 4, 2025
@kunalspathak
Copy link
Contributor Author

/azp run runtime-coreclr gcstress-extra

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@a74nh
Copy link
Contributor

a74nh commented Mar 5, 2025

This one uses a different template?

coreclr linux arm64 Checked no_tiered_compilation @ (Ubuntu.2004.Arm64.Open)[email protected]/dotnet-buildtools/prereqs:ubuntu-20.04-helix-arm64v8
    - _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_GatherVector_Bases_double_ulong()

I was looking at SveGatherVectorFirstFaultingVectorBases.template.

So there might be more to fix.

@kunalspathak
Copy link
Contributor Author

SveGatherVectorFirstFaultingVectorBases.template.

This doesn't have problems because it is operating on correct size. Latest gcstress runs are passing.

@kunalspathak
Copy link
Contributor Author

@amanasifkhalid @dotnet/jit-contrib

amanasifkhalid
amanasifkhalid approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@amanasifkhalid amanasifkhalid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I'm guessing the Unsafe.SizeOf result was including padding or something?

@kunalspathak
Copy link
Contributor Author

I'm guessing the Unsafe.SizeOf result was including padding or something?

not really. The destination was sometimes just 4 bytes (e.g. array of ushort with 2 elements), while the Unsafe.SizeOf()` was 16 bytes, so we would overwrite past the destination.

@kunalspathak kunalspathak merged commit 28facae into dotnet:main Mar 5, 2025
82 of 88 checks passed
EgorBo
EgorBo reviewed Mar 5, 2025
View reviewed changes
src/tests/JIT/HardwareIntrinsics/Arm/Shared/SveLoadVectorFirstFaultingTest.template
Unsafe.CopyBlockUnaligned(ref Unsafe.As<{Op2BaseType}, byte>(ref inArray[0]), ref Unsafe.AsRef<byte>(firstOp), (uint)Unsafe.SizeOf<{RetVectorType}<{Op2BaseType}>>());
Unsafe.CopyBlockUnaligned(ref Unsafe.As<{RetBaseType}, byte>(ref outArray[0]), ref Unsafe.AsRef<byte>(result), (uint)Unsafe.SizeOf<{RetVectorType}<{RetBaseType}>>());
Unsafe.CopyBlockUnaligned(ref Unsafe.As<{Op2BaseType}, byte>(ref inArray[0]), ref Unsafe.AsRef<byte>(firstOp), (uint)(sizeof({Op2BaseType}) * RetElementCount));
Unsafe.CopyBlockUnaligned(ref Unsafe.As<{RetBaseType}, byte>(ref outArray[0]), ref Unsafe.AsRef<byte>(result), (uint)(sizeof({RetBaseType}) * RetElementCount));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why the test should've used the safe Span.CopyTo 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree. There are 1000+ places where we are using Unsafe.CopyBlockUnaligned and should change to something reliable.

@EgorBo EgorBo mentioned this pull request Mar 14, 2025
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@EgorBo EgorBo EgorBo left review comments

Copilot code review Copilot Copilot left review comments

+1 more reviewer

@amanasifkhalid amanasifkhalid amanasifkhalid approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kunalspathak kunalspathak

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Sve: *FirstFaulting* are mostly failing in gcstress runs

4 participants

@kunalspathak @a74nh @EgorBo @amanasifkhalid