Skip to content

[mono][aot] Avoid use of local buffer that could overflow #118985

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
BrzVlad merged 2 commits into from
Aug 29, 2025
Merged

[mono][aot] Avoid use of local buffer that could overflow #118985

BrzVlad merged 2 commits into from
Aug 29, 2025

Conversation

@BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Aug 22, 2025 •
edited
Loading

Copilot AI review requested due to automatic review settings August 22, 2025 10:26
Copilot AI reviewed Aug 22, 2025
View reviewed changes
Copy link
Contributor

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.

Pull Request Overview

This PR eliminates a potential buffer overflow vulnerability in the Mono AOT compiler by removing the use of a local buffer for symbol name handling. The change replaces sprintf operations that could exceed buffer bounds with direct usage of existing symbol strings.

Key Changes:

  • Removes local symbol buffer that was vulnerable to overflow
  • Eliminates unnecessary sprintf calls for symbol name copying
  • Uses acfg->got_symbol directly instead of copying to local buffer

@BrzVlad BrzVlad requested a review from simonrozsival August 22, 2025 10:27
@BrzVlad
Copy link
Member Author

BrzVlad commented Aug 22, 2025

@simonrozsival Looked over other uses of MAX_SYMBOL_SIZE throughout aot-compiler.c but couldn't find any other location that could potentially overflow. Thus decided against bumping the max symbol size.

vitek-karas
vitek-karas approved these changes Aug 22, 2025
View reviewed changes
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

Looks good.

I don't understand why the code copied the strings before - makes no sense...

@BrzVlad
Copy link
Member Author

BrzVlad commented Aug 22, 2025

Looks good.

I don't understand why the code copied the strings before - makes no sense...

Long time ago AOT code had a pattern of always using strdup_printf for creating symbol names. I believe this was so that all pointers to strings were free-able. Code was refactored around, optimization was added in order to use local buffers instead, and this redundancy was leftover.

This was referenced Aug 22, 2025
Open
Open
Open
Open
Closed
@vitek-karas vitek-karas added this to the 10.0.0 milestone Aug 26, 2025
@vitek-karas
Copy link
Member

vitek-karas commented Aug 26, 2025

This has to go to .NET 10 - we should prioritize this.

@build-analysis build-analysis bot mentioned this pull request Aug 27, 2025
Open
3 tasks
@BrzVlad
Copy link
Member Author

BrzVlad commented Aug 28, 2025 •
edited
Loading

This seems to consistently fail on windows WasmBuildTests even though it makes zero sense (it seems it only fails on my PR?). The change is trivial and it shouldn't even be hit on wasm, since the aot-compiler should only run in llvmonly mode. I tried reproducing locally both on macos and on windows but I didn't notice anything extraordinary. With or without my change there are failures. Any thoughts about this failure, whether this is safe to go in ? @lewing @pavelsavara For example https://helixr1107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-118985-merge-18337050e6e64ed191/NoFingerprint-ST-Wasm.Build.Tests/1/console.256ab69f.log?helixlogtype=result , currently there is not even any info https://helix.dot.net/api/2019-06-17/jobs/08f65c11-4d59-420d-b4d9-1294fb2af40b/workitems/NoWebcil-ST-Wasm.Build.NativeRebuild.Tests.OptimizationFlagChangeTests/console

@build-analysis build-analysis bot mentioned this pull request Aug 28, 2025
Open
@pavelsavara
Copy link
Member

pavelsavara commented Aug 29, 2025

Any thoughts about this failure, whether this is safe to go in ?

Yes, timeouts in WasmBasicTest are known. They are not easy to fix, sorry.

@BrzVlad BrzVlad merged commit 2562b73 into dotnet:main Aug 29, 2025
81 of 85 checks passed
@BrzVlad
Copy link
Member Author

BrzVlad commented Aug 29, 2025

/backport to release/10.0

@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2025

Started backporting to release/10.0: https://github.com/dotnet/runtime/actions/runs/17322274062

@github-actions github-actions bot mentioned this pull request Aug 29, 2025
Merged
4 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Sep 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot Copilot left review comments

@simonrozsival simonrozsival simonrozsival approved these changes

@steveisok steveisok steveisok approved these changes

@vitek-karas vitek-karas vitek-karas approved these changes

Assignees

@BrzVlad BrzVlad

Labels

area-Codegen-AOT-mono

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@BrzVlad @vitek-karas @pavelsavara @simonrozsival @steveisok @lewing