Replace more strcpy with safe string copy - #131308

Open
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy
Open

Replace more strcpy with safe string copy#131308
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy

Conversation

@am11

@am11am11 commented Jul 24, 2026

Copy link
Copy Markdown
Member

Follow up #130547.

@am11am11 mentioned this pull request Jul 24, 2026
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jul 24, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 6 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

Comment threadsrc/coreclr/gc/unix/cgroup.cpp Outdated
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

@jkotasjkotasJul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What guarantees that parent_directory_end has enough space? This pattern is likely to trigger static analyzers.

(not commenting on all places with this issue)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It's same guarantees as before; none.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The PR title is "Replace more strcpy with safe string copy". Why is this safe string copy then?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

strcpy -> memcpy/strcpy_s, because strcpy is being flagged by clang on OpenBSD warning: strcpy() is almost always misused, please use strlcpy().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think Jan's point is that if we're changing this code at all, we should change it to something better, not to something that has the same problem and is somewhat harder to read than the original.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Not sure how it is harder to read than original given #130547 is merged?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was fine with #130547 since it was an open-coded strdup, no length arithmetic, length is computed on one line and used a few lines later in a very straightforward way. In hindsight, I should have suggested to just use strdup instead of open coding it. We use strdup in number of other places. Could you please replace it with strdup as part of these changes?

This code is not a straightforward like that.

We have SafeStringCopy in pal_utilities.h to solve this problem. Can we promote it or some variant of it to be usable everywhere? Would it be possible to standardize on strcpy_s and polyfill strcpy_s when it is not available - do we build on any platforms like that?

@am11
am11force-pushed the chore/mop-strcpy branch from 6a2d59d to c0f7062CompareJuly 24, 2026 11:29
@am11
am11force-pushed the chore/mop-strcpy branch from c0f7062 to 7ea1219CompareJuly 24, 2026 12:00
@am11
am11 requested a review from jkoritzinskyJuly 24, 2026 15:50
@am11

am11 commented Jul 24, 2026

Copy link
Copy Markdown
MemberAuthor

BA failure is #131256.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 continues the effort to eliminate strcpy usage across CoreCLR, NativeAOT, and related tools by replacing it with length-aware copies (primarily memcpy) and minor refactors to compute/carry explicit buffer lengths.

Changes:

  • Replaced multiple strcpy call sites with memcpy (often using strlen(...) + 1) across VM/JIT/GC/NativeAOT/host/tooling code.
  • Introduced explicit length variables in a few places to make copy sizes clearer and avoid repeated strlen in allocation/copy sequences.
  • Added a couple of small robustness tweaks (e.g., guarding malloc results before copying in corerun.cpp).

Reviewed changes

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

Show a summary per file
FileDescription
src/coreclr/vm/gdbjit.cppReplaces strcpy with memcpy in GDB JIT debug-info string materialization paths.
src/coreclr/utilcode/debug.cppReplaces strcpy with memcpy when building the assert expression+stacktrace display buffer.
src/coreclr/utilcode/check.cppUses memcpy for copying dynamically allocated CHECK failure messages.
src/coreclr/nativeaot/Runtime/unix/PalUnix.cppUses computed length + memcpy in PalCopyTCharAsChar.
src/coreclr/nativeaot/Runtime/unix/cgroupcpu.cppReworks cgroup path construction to use explicit lengths and memcpy instead of strcpy/strcat.
src/coreclr/nativeaot/Runtime/RhConfig.cppRefactors env-var name construction and switches embedded string copies to memcpy.
src/coreclr/nativeaot/Runtime/clrgc.enabled.cppReplaces strcpy concatenation with length-based memcpy copies.
src/coreclr/jit/fgdiagnostic.cppReplaces strcpy with memcpy in escape-substitution copying.
src/coreclr/interpreter/methodset.cppCopies config string via memcpy instead of strcpy.
src/coreclr/inc/outstring.hUpdates <string.h> include comment (no longer calls out strcpy).
src/coreclr/ildasm/dres.cppReplaces strcat/strcpy usage with manual memcpy for indentation/string building in resource dumping.
src/coreclr/ilasm/method.hppUpdates commented-out code to reference strcpy_s.
src/coreclr/hosts/corerun/corerun.cppUses length-based memcpy and guards malloc before copying core paths.
src/coreclr/gc/unix/cgroup.cppUses mount-length caching + memcpy for initial copy and replaces a strcpy with memcpy.

Comment threadsrc/coreclr/utilcode/debug.cpp Outdated
Comment threadsrc/coreclr/ildasm/dres.cpp Outdated
Comment threadsrc/coreclr/interpreter/methodset.cpp
am11and others added 2 commits August 13, 2026 10:07
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we gain anything here when the source is a string constant?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-VM-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@am11@jkotas@janvorli@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Replace more strcpy with safe string copy - #131308

Open
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy
Open

Replace more strcpy with safe string copy#131308
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy

Conversation

@am11

@am11am11 commented Jul 24, 2026

Copy link
Copy Markdown
Member

Follow up #130547.

@am11am11 mentioned this pull request Jul 24, 2026
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jul 24, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 6 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

Comment threadsrc/coreclr/gc/unix/cgroup.cpp Outdated
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

@jkotasjkotasJul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What guarantees that parent_directory_end has enough space? This pattern is likely to trigger static analyzers.

(not commenting on all places with this issue)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It's same guarantees as before; none.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The PR title is "Replace more strcpy with safe string copy". Why is this safe string copy then?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

strcpy -> memcpy/strcpy_s, because strcpy is being flagged by clang on OpenBSD warning: strcpy() is almost always misused, please use strlcpy().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think Jan's point is that if we're changing this code at all, we should change it to something better, not to something that has the same problem and is somewhat harder to read than the original.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Not sure how it is harder to read than original given #130547 is merged?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was fine with #130547 since it was an open-coded strdup, no length arithmetic, length is computed on one line and used a few lines later in a very straightforward way. In hindsight, I should have suggested to just use strdup instead of open coding it. We use strdup in number of other places. Could you please replace it with strdup as part of these changes?

This code is not a straightforward like that.

We have SafeStringCopy in pal_utilities.h to solve this problem. Can we promote it or some variant of it to be usable everywhere? Would it be possible to standardize on strcpy_s and polyfill strcpy_s when it is not available - do we build on any platforms like that?

@am11
am11force-pushed the chore/mop-strcpy branch from 6a2d59d to c0f7062CompareJuly 24, 2026 11:29
@am11
am11force-pushed the chore/mop-strcpy branch from c0f7062 to 7ea1219CompareJuly 24, 2026 12:00
@am11
am11 requested a review from jkoritzinskyJuly 24, 2026 15:50
@am11

am11 commented Jul 24, 2026

Copy link
Copy Markdown
MemberAuthor

BA failure is #131256.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 continues the effort to eliminate strcpy usage across CoreCLR, NativeAOT, and related tools by replacing it with length-aware copies (primarily memcpy) and minor refactors to compute/carry explicit buffer lengths.

Changes:

  • Replaced multiple strcpy call sites with memcpy (often using strlen(...) + 1) across VM/JIT/GC/NativeAOT/host/tooling code.
  • Introduced explicit length variables in a few places to make copy sizes clearer and avoid repeated strlen in allocation/copy sequences.
  • Added a couple of small robustness tweaks (e.g., guarding malloc results before copying in corerun.cpp).

Reviewed changes

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

Show a summary per file
FileDescription
src/coreclr/vm/gdbjit.cppReplaces strcpy with memcpy in GDB JIT debug-info string materialization paths.
src/coreclr/utilcode/debug.cppReplaces strcpy with memcpy when building the assert expression+stacktrace display buffer.
src/coreclr/utilcode/check.cppUses memcpy for copying dynamically allocated CHECK failure messages.
src/coreclr/nativeaot/Runtime/unix/PalUnix.cppUses computed length + memcpy in PalCopyTCharAsChar.
src/coreclr/nativeaot/Runtime/unix/cgroupcpu.cppReworks cgroup path construction to use explicit lengths and memcpy instead of strcpy/strcat.
src/coreclr/nativeaot/Runtime/RhConfig.cppRefactors env-var name construction and switches embedded string copies to memcpy.
src/coreclr/nativeaot/Runtime/clrgc.enabled.cppReplaces strcpy concatenation with length-based memcpy copies.
src/coreclr/jit/fgdiagnostic.cppReplaces strcpy with memcpy in escape-substitution copying.
src/coreclr/interpreter/methodset.cppCopies config string via memcpy instead of strcpy.
src/coreclr/inc/outstring.hUpdates <string.h> include comment (no longer calls out strcpy).
src/coreclr/ildasm/dres.cppReplaces strcat/strcpy usage with manual memcpy for indentation/string building in resource dumping.
src/coreclr/ilasm/method.hppUpdates commented-out code to reference strcpy_s.
src/coreclr/hosts/corerun/corerun.cppUses length-based memcpy and guards malloc before copying core paths.
src/coreclr/gc/unix/cgroup.cppUses mount-length caching + memcpy for initial copy and replaces a strcpy with memcpy.

Comment threadsrc/coreclr/utilcode/debug.cpp Outdated
Comment threadsrc/coreclr/ildasm/dres.cpp Outdated
Comment threadsrc/coreclr/interpreter/methodset.cpp
am11and others added 2 commits August 13, 2026 10:07
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we gain anything here when the source is a string constant?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-VM-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@am11@jkotas@janvorli@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Replace more strcpy with safe string copy - #131308

Open
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy
Open

Replace more strcpy with safe string copy#131308
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy

Conversation

@am11

@am11am11 commented Jul 24, 2026

Copy link
Copy Markdown
Member

Follow up #130547.

@am11am11 mentioned this pull request Jul 24, 2026
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jul 24, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 6 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

Comment threadsrc/coreclr/gc/unix/cgroup.cpp Outdated
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

@jkotasjkotasJul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What guarantees that parent_directory_end has enough space? This pattern is likely to trigger static analyzers.

(not commenting on all places with this issue)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It's same guarantees as before; none.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The PR title is "Replace more strcpy with safe string copy". Why is this safe string copy then?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

strcpy -> memcpy/strcpy_s, because strcpy is being flagged by clang on OpenBSD warning: strcpy() is almost always misused, please use strlcpy().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think Jan's point is that if we're changing this code at all, we should change it to something better, not to something that has the same problem and is somewhat harder to read than the original.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Not sure how it is harder to read than original given #130547 is merged?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was fine with #130547 since it was an open-coded strdup, no length arithmetic, length is computed on one line and used a few lines later in a very straightforward way. In hindsight, I should have suggested to just use strdup instead of open coding it. We use strdup in number of other places. Could you please replace it with strdup as part of these changes?

This code is not a straightforward like that.

We have SafeStringCopy in pal_utilities.h to solve this problem. Can we promote it or some variant of it to be usable everywhere? Would it be possible to standardize on strcpy_s and polyfill strcpy_s when it is not available - do we build on any platforms like that?

@am11
am11force-pushed the chore/mop-strcpy branch from 6a2d59d to c0f7062CompareJuly 24, 2026 11:29
@am11
am11force-pushed the chore/mop-strcpy branch from c0f7062 to 7ea1219CompareJuly 24, 2026 12:00
@am11
am11 requested a review from jkoritzinskyJuly 24, 2026 15:50
@am11

am11 commented Jul 24, 2026

Copy link
Copy Markdown
MemberAuthor

BA failure is #131256.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 continues the effort to eliminate strcpy usage across CoreCLR, NativeAOT, and related tools by replacing it with length-aware copies (primarily memcpy) and minor refactors to compute/carry explicit buffer lengths.

Changes:

  • Replaced multiple strcpy call sites with memcpy (often using strlen(...) + 1) across VM/JIT/GC/NativeAOT/host/tooling code.
  • Introduced explicit length variables in a few places to make copy sizes clearer and avoid repeated strlen in allocation/copy sequences.
  • Added a couple of small robustness tweaks (e.g., guarding malloc results before copying in corerun.cpp).

Reviewed changes

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

Show a summary per file
FileDescription
src/coreclr/vm/gdbjit.cppReplaces strcpy with memcpy in GDB JIT debug-info string materialization paths.
src/coreclr/utilcode/debug.cppReplaces strcpy with memcpy when building the assert expression+stacktrace display buffer.
src/coreclr/utilcode/check.cppUses memcpy for copying dynamically allocated CHECK failure messages.
src/coreclr/nativeaot/Runtime/unix/PalUnix.cppUses computed length + memcpy in PalCopyTCharAsChar.
src/coreclr/nativeaot/Runtime/unix/cgroupcpu.cppReworks cgroup path construction to use explicit lengths and memcpy instead of strcpy/strcat.
src/coreclr/nativeaot/Runtime/RhConfig.cppRefactors env-var name construction and switches embedded string copies to memcpy.
src/coreclr/nativeaot/Runtime/clrgc.enabled.cppReplaces strcpy concatenation with length-based memcpy copies.
src/coreclr/jit/fgdiagnostic.cppReplaces strcpy with memcpy in escape-substitution copying.
src/coreclr/interpreter/methodset.cppCopies config string via memcpy instead of strcpy.
src/coreclr/inc/outstring.hUpdates <string.h> include comment (no longer calls out strcpy).
src/coreclr/ildasm/dres.cppReplaces strcat/strcpy usage with manual memcpy for indentation/string building in resource dumping.
src/coreclr/ilasm/method.hppUpdates commented-out code to reference strcpy_s.
src/coreclr/hosts/corerun/corerun.cppUses length-based memcpy and guards malloc before copying core paths.
src/coreclr/gc/unix/cgroup.cppUses mount-length caching + memcpy for initial copy and replaces a strcpy with memcpy.

Comment threadsrc/coreclr/utilcode/debug.cpp Outdated
Comment threadsrc/coreclr/ildasm/dres.cpp Outdated
Comment threadsrc/coreclr/interpreter/methodset.cpp
am11and others added 2 commits August 13, 2026 10:07
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we gain anything here when the source is a string constant?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-VM-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@am11@jkotas@janvorli@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Replace more strcpy with safe string copy - #131308

Open
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy
Open

Replace more strcpy with safe string copy#131308
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy

Conversation

@am11

@am11am11 commented Jul 24, 2026

Copy link
Copy Markdown
Member

Follow up #130547.

@am11am11 mentioned this pull request Jul 24, 2026
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jul 24, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 6 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

Comment threadsrc/coreclr/gc/unix/cgroup.cpp Outdated
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

@jkotasjkotasJul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What guarantees that parent_directory_end has enough space? This pattern is likely to trigger static analyzers.

(not commenting on all places with this issue)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It's same guarantees as before; none.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The PR title is "Replace more strcpy with safe string copy". Why is this safe string copy then?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

strcpy -> memcpy/strcpy_s, because strcpy is being flagged by clang on OpenBSD warning: strcpy() is almost always misused, please use strlcpy().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think Jan's point is that if we're changing this code at all, we should change it to something better, not to something that has the same problem and is somewhat harder to read than the original.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Not sure how it is harder to read than original given #130547 is merged?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was fine with #130547 since it was an open-coded strdup, no length arithmetic, length is computed on one line and used a few lines later in a very straightforward way. In hindsight, I should have suggested to just use strdup instead of open coding it. We use strdup in number of other places. Could you please replace it with strdup as part of these changes?

This code is not a straightforward like that.

We have SafeStringCopy in pal_utilities.h to solve this problem. Can we promote it or some variant of it to be usable everywhere? Would it be possible to standardize on strcpy_s and polyfill strcpy_s when it is not available - do we build on any platforms like that?

@am11
am11force-pushed the chore/mop-strcpy branch from 6a2d59d to c0f7062CompareJuly 24, 2026 11:29
@am11
am11force-pushed the chore/mop-strcpy branch from c0f7062 to 7ea1219CompareJuly 24, 2026 12:00
@am11
am11 requested a review from jkoritzinskyJuly 24, 2026 15:50
@am11

am11 commented Jul 24, 2026

Copy link
Copy Markdown
MemberAuthor

BA failure is #131256.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 continues the effort to eliminate strcpy usage across CoreCLR, NativeAOT, and related tools by replacing it with length-aware copies (primarily memcpy) and minor refactors to compute/carry explicit buffer lengths.

Changes:

  • Replaced multiple strcpy call sites with memcpy (often using strlen(...) + 1) across VM/JIT/GC/NativeAOT/host/tooling code.
  • Introduced explicit length variables in a few places to make copy sizes clearer and avoid repeated strlen in allocation/copy sequences.
  • Added a couple of small robustness tweaks (e.g., guarding malloc results before copying in corerun.cpp).

Reviewed changes

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

Show a summary per file
FileDescription
src/coreclr/vm/gdbjit.cppReplaces strcpy with memcpy in GDB JIT debug-info string materialization paths.
src/coreclr/utilcode/debug.cppReplaces strcpy with memcpy when building the assert expression+stacktrace display buffer.
src/coreclr/utilcode/check.cppUses memcpy for copying dynamically allocated CHECK failure messages.
src/coreclr/nativeaot/Runtime/unix/PalUnix.cppUses computed length + memcpy in PalCopyTCharAsChar.
src/coreclr/nativeaot/Runtime/unix/cgroupcpu.cppReworks cgroup path construction to use explicit lengths and memcpy instead of strcpy/strcat.
src/coreclr/nativeaot/Runtime/RhConfig.cppRefactors env-var name construction and switches embedded string copies to memcpy.
src/coreclr/nativeaot/Runtime/clrgc.enabled.cppReplaces strcpy concatenation with length-based memcpy copies.
src/coreclr/jit/fgdiagnostic.cppReplaces strcpy with memcpy in escape-substitution copying.
src/coreclr/interpreter/methodset.cppCopies config string via memcpy instead of strcpy.
src/coreclr/inc/outstring.hUpdates <string.h> include comment (no longer calls out strcpy).
src/coreclr/ildasm/dres.cppReplaces strcat/strcpy usage with manual memcpy for indentation/string building in resource dumping.
src/coreclr/ilasm/method.hppUpdates commented-out code to reference strcpy_s.
src/coreclr/hosts/corerun/corerun.cppUses length-based memcpy and guards malloc before copying core paths.
src/coreclr/gc/unix/cgroup.cppUses mount-length caching + memcpy for initial copy and replaces a strcpy with memcpy.

Comment threadsrc/coreclr/utilcode/debug.cpp Outdated
Comment threadsrc/coreclr/ildasm/dres.cpp Outdated
Comment threadsrc/coreclr/interpreter/methodset.cpp
am11and others added 2 commits August 13, 2026 10:07
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we gain anything here when the source is a string constant?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-VM-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@am11@jkotas@janvorli@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Replace more strcpy with safe string copy - #131308

Open
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy
Open

Replace more strcpy with safe string copy#131308
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy

Conversation

@am11

@am11am11 commented Jul 24, 2026

Copy link
Copy Markdown
Member

Follow up #130547.

@am11am11 mentioned this pull request Jul 24, 2026
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jul 24, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 6 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

Comment threadsrc/coreclr/gc/unix/cgroup.cpp Outdated
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

@jkotasjkotasJul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What guarantees that parent_directory_end has enough space? This pattern is likely to trigger static analyzers.

(not commenting on all places with this issue)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It's same guarantees as before; none.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The PR title is "Replace more strcpy with safe string copy". Why is this safe string copy then?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

strcpy -> memcpy/strcpy_s, because strcpy is being flagged by clang on OpenBSD warning: strcpy() is almost always misused, please use strlcpy().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think Jan's point is that if we're changing this code at all, we should change it to something better, not to something that has the same problem and is somewhat harder to read than the original.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Not sure how it is harder to read than original given #130547 is merged?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was fine with #130547 since it was an open-coded strdup, no length arithmetic, length is computed on one line and used a few lines later in a very straightforward way. In hindsight, I should have suggested to just use strdup instead of open coding it. We use strdup in number of other places. Could you please replace it with strdup as part of these changes?

This code is not a straightforward like that.

We have SafeStringCopy in pal_utilities.h to solve this problem. Can we promote it or some variant of it to be usable everywhere? Would it be possible to standardize on strcpy_s and polyfill strcpy_s when it is not available - do we build on any platforms like that?

@am11
am11force-pushed the chore/mop-strcpy branch from 6a2d59d to c0f7062CompareJuly 24, 2026 11:29
@am11
am11force-pushed the chore/mop-strcpy branch from c0f7062 to 7ea1219CompareJuly 24, 2026 12:00
@am11
am11 requested a review from jkoritzinskyJuly 24, 2026 15:50
@am11

am11 commented Jul 24, 2026

Copy link
Copy Markdown
MemberAuthor

BA failure is #131256.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 continues the effort to eliminate strcpy usage across CoreCLR, NativeAOT, and related tools by replacing it with length-aware copies (primarily memcpy) and minor refactors to compute/carry explicit buffer lengths.

Changes:

  • Replaced multiple strcpy call sites with memcpy (often using strlen(...) + 1) across VM/JIT/GC/NativeAOT/host/tooling code.
  • Introduced explicit length variables in a few places to make copy sizes clearer and avoid repeated strlen in allocation/copy sequences.
  • Added a couple of small robustness tweaks (e.g., guarding malloc results before copying in corerun.cpp).

Reviewed changes

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

Show a summary per file
FileDescription
src/coreclr/vm/gdbjit.cppReplaces strcpy with memcpy in GDB JIT debug-info string materialization paths.
src/coreclr/utilcode/debug.cppReplaces strcpy with memcpy when building the assert expression+stacktrace display buffer.
src/coreclr/utilcode/check.cppUses memcpy for copying dynamically allocated CHECK failure messages.
src/coreclr/nativeaot/Runtime/unix/PalUnix.cppUses computed length + memcpy in PalCopyTCharAsChar.
src/coreclr/nativeaot/Runtime/unix/cgroupcpu.cppReworks cgroup path construction to use explicit lengths and memcpy instead of strcpy/strcat.
src/coreclr/nativeaot/Runtime/RhConfig.cppRefactors env-var name construction and switches embedded string copies to memcpy.
src/coreclr/nativeaot/Runtime/clrgc.enabled.cppReplaces strcpy concatenation with length-based memcpy copies.
src/coreclr/jit/fgdiagnostic.cppReplaces strcpy with memcpy in escape-substitution copying.
src/coreclr/interpreter/methodset.cppCopies config string via memcpy instead of strcpy.
src/coreclr/inc/outstring.hUpdates <string.h> include comment (no longer calls out strcpy).
src/coreclr/ildasm/dres.cppReplaces strcat/strcpy usage with manual memcpy for indentation/string building in resource dumping.
src/coreclr/ilasm/method.hppUpdates commented-out code to reference strcpy_s.
src/coreclr/hosts/corerun/corerun.cppUses length-based memcpy and guards malloc before copying core paths.
src/coreclr/gc/unix/cgroup.cppUses mount-length caching + memcpy for initial copy and replaces a strcpy with memcpy.

Comment threadsrc/coreclr/utilcode/debug.cpp Outdated
Comment threadsrc/coreclr/ildasm/dres.cpp Outdated
Comment threadsrc/coreclr/interpreter/methodset.cpp
am11and others added 2 commits August 13, 2026 10:07
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we gain anything here when the source is a string constant?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-VM-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@am11@jkotas@janvorli@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Replace more strcpy with safe string copy - #131308

Open
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy
Open

Replace more strcpy with safe string copy#131308
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy

Conversation

@am11

@am11am11 commented Jul 24, 2026

Copy link
Copy Markdown
Member

Follow up #130547.

@am11am11 mentioned this pull request Jul 24, 2026
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jul 24, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 6 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

Comment threadsrc/coreclr/gc/unix/cgroup.cpp Outdated
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

@jkotasjkotasJul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What guarantees that parent_directory_end has enough space? This pattern is likely to trigger static analyzers.

(not commenting on all places with this issue)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It's same guarantees as before; none.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The PR title is "Replace more strcpy with safe string copy". Why is this safe string copy then?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

strcpy -> memcpy/strcpy_s, because strcpy is being flagged by clang on OpenBSD warning: strcpy() is almost always misused, please use strlcpy().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think Jan's point is that if we're changing this code at all, we should change it to something better, not to something that has the same problem and is somewhat harder to read than the original.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Not sure how it is harder to read than original given #130547 is merged?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was fine with #130547 since it was an open-coded strdup, no length arithmetic, length is computed on one line and used a few lines later in a very straightforward way. In hindsight, I should have suggested to just use strdup instead of open coding it. We use strdup in number of other places. Could you please replace it with strdup as part of these changes?

This code is not a straightforward like that.

We have SafeStringCopy in pal_utilities.h to solve this problem. Can we promote it or some variant of it to be usable everywhere? Would it be possible to standardize on strcpy_s and polyfill strcpy_s when it is not available - do we build on any platforms like that?

@am11
am11force-pushed the chore/mop-strcpy branch from 6a2d59d to c0f7062CompareJuly 24, 2026 11:29
@am11
am11force-pushed the chore/mop-strcpy branch from c0f7062 to 7ea1219CompareJuly 24, 2026 12:00
@am11
am11 requested a review from jkoritzinskyJuly 24, 2026 15:50
@am11

am11 commented Jul 24, 2026

Copy link
Copy Markdown
MemberAuthor

BA failure is #131256.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 continues the effort to eliminate strcpy usage across CoreCLR, NativeAOT, and related tools by replacing it with length-aware copies (primarily memcpy) and minor refactors to compute/carry explicit buffer lengths.

Changes:

  • Replaced multiple strcpy call sites with memcpy (often using strlen(...) + 1) across VM/JIT/GC/NativeAOT/host/tooling code.
  • Introduced explicit length variables in a few places to make copy sizes clearer and avoid repeated strlen in allocation/copy sequences.
  • Added a couple of small robustness tweaks (e.g., guarding malloc results before copying in corerun.cpp).

Reviewed changes

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

Show a summary per file
FileDescription
src/coreclr/vm/gdbjit.cppReplaces strcpy with memcpy in GDB JIT debug-info string materialization paths.
src/coreclr/utilcode/debug.cppReplaces strcpy with memcpy when building the assert expression+stacktrace display buffer.
src/coreclr/utilcode/check.cppUses memcpy for copying dynamically allocated CHECK failure messages.
src/coreclr/nativeaot/Runtime/unix/PalUnix.cppUses computed length + memcpy in PalCopyTCharAsChar.
src/coreclr/nativeaot/Runtime/unix/cgroupcpu.cppReworks cgroup path construction to use explicit lengths and memcpy instead of strcpy/strcat.
src/coreclr/nativeaot/Runtime/RhConfig.cppRefactors env-var name construction and switches embedded string copies to memcpy.
src/coreclr/nativeaot/Runtime/clrgc.enabled.cppReplaces strcpy concatenation with length-based memcpy copies.
src/coreclr/jit/fgdiagnostic.cppReplaces strcpy with memcpy in escape-substitution copying.
src/coreclr/interpreter/methodset.cppCopies config string via memcpy instead of strcpy.
src/coreclr/inc/outstring.hUpdates <string.h> include comment (no longer calls out strcpy).
src/coreclr/ildasm/dres.cppReplaces strcat/strcpy usage with manual memcpy for indentation/string building in resource dumping.
src/coreclr/ilasm/method.hppUpdates commented-out code to reference strcpy_s.
src/coreclr/hosts/corerun/corerun.cppUses length-based memcpy and guards malloc before copying core paths.
src/coreclr/gc/unix/cgroup.cppUses mount-length caching + memcpy for initial copy and replaces a strcpy with memcpy.

Comment threadsrc/coreclr/utilcode/debug.cpp Outdated
Comment threadsrc/coreclr/ildasm/dres.cpp Outdated
Comment threadsrc/coreclr/interpreter/methodset.cpp
am11and others added 2 commits August 13, 2026 10:07
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we gain anything here when the source is a string constant?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-VM-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@am11@jkotas@janvorli@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Replace more strcpy with safe string copy - #131308

Open
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy
Open

Replace more strcpy with safe string copy#131308
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy

Conversation

@am11

@am11am11 commented Jul 24, 2026

Copy link
Copy Markdown
Member

Follow up #130547.

@am11am11 mentioned this pull request Jul 24, 2026
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jul 24, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 6 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

Comment threadsrc/coreclr/gc/unix/cgroup.cpp Outdated
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

@jkotasjkotasJul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What guarantees that parent_directory_end has enough space? This pattern is likely to trigger static analyzers.

(not commenting on all places with this issue)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It's same guarantees as before; none.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The PR title is "Replace more strcpy with safe string copy". Why is this safe string copy then?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

strcpy -> memcpy/strcpy_s, because strcpy is being flagged by clang on OpenBSD warning: strcpy() is almost always misused, please use strlcpy().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think Jan's point is that if we're changing this code at all, we should change it to something better, not to something that has the same problem and is somewhat harder to read than the original.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Not sure how it is harder to read than original given #130547 is merged?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was fine with #130547 since it was an open-coded strdup, no length arithmetic, length is computed on one line and used a few lines later in a very straightforward way. In hindsight, I should have suggested to just use strdup instead of open coding it. We use strdup in number of other places. Could you please replace it with strdup as part of these changes?

This code is not a straightforward like that.

We have SafeStringCopy in pal_utilities.h to solve this problem. Can we promote it or some variant of it to be usable everywhere? Would it be possible to standardize on strcpy_s and polyfill strcpy_s when it is not available - do we build on any platforms like that?

@am11
am11force-pushed the chore/mop-strcpy branch from 6a2d59d to c0f7062CompareJuly 24, 2026 11:29
@am11
am11force-pushed the chore/mop-strcpy branch from c0f7062 to 7ea1219CompareJuly 24, 2026 12:00
@am11
am11 requested a review from jkoritzinskyJuly 24, 2026 15:50
@am11

am11 commented Jul 24, 2026

Copy link
Copy Markdown
MemberAuthor

BA failure is #131256.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 continues the effort to eliminate strcpy usage across CoreCLR, NativeAOT, and related tools by replacing it with length-aware copies (primarily memcpy) and minor refactors to compute/carry explicit buffer lengths.

Changes:

  • Replaced multiple strcpy call sites with memcpy (often using strlen(...) + 1) across VM/JIT/GC/NativeAOT/host/tooling code.
  • Introduced explicit length variables in a few places to make copy sizes clearer and avoid repeated strlen in allocation/copy sequences.
  • Added a couple of small robustness tweaks (e.g., guarding malloc results before copying in corerun.cpp).

Reviewed changes

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

Show a summary per file
FileDescription
src/coreclr/vm/gdbjit.cppReplaces strcpy with memcpy in GDB JIT debug-info string materialization paths.
src/coreclr/utilcode/debug.cppReplaces strcpy with memcpy when building the assert expression+stacktrace display buffer.
src/coreclr/utilcode/check.cppUses memcpy for copying dynamically allocated CHECK failure messages.
src/coreclr/nativeaot/Runtime/unix/PalUnix.cppUses computed length + memcpy in PalCopyTCharAsChar.
src/coreclr/nativeaot/Runtime/unix/cgroupcpu.cppReworks cgroup path construction to use explicit lengths and memcpy instead of strcpy/strcat.
src/coreclr/nativeaot/Runtime/RhConfig.cppRefactors env-var name construction and switches embedded string copies to memcpy.
src/coreclr/nativeaot/Runtime/clrgc.enabled.cppReplaces strcpy concatenation with length-based memcpy copies.
src/coreclr/jit/fgdiagnostic.cppReplaces strcpy with memcpy in escape-substitution copying.
src/coreclr/interpreter/methodset.cppCopies config string via memcpy instead of strcpy.
src/coreclr/inc/outstring.hUpdates <string.h> include comment (no longer calls out strcpy).
src/coreclr/ildasm/dres.cppReplaces strcat/strcpy usage with manual memcpy for indentation/string building in resource dumping.
src/coreclr/ilasm/method.hppUpdates commented-out code to reference strcpy_s.
src/coreclr/hosts/corerun/corerun.cppUses length-based memcpy and guards malloc before copying core paths.
src/coreclr/gc/unix/cgroup.cppUses mount-length caching + memcpy for initial copy and replaces a strcpy with memcpy.

Comment threadsrc/coreclr/utilcode/debug.cpp Outdated
Comment threadsrc/coreclr/ildasm/dres.cpp Outdated
Comment threadsrc/coreclr/interpreter/methodset.cpp
am11and others added 2 commits August 13, 2026 10:07
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we gain anything here when the source is a string constant?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-VM-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@am11@jkotas@janvorli@MichalStrehovsky
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Replace more strcpy with safe string copy - #131308

Open
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy
Open

Replace more strcpy with safe string copy#131308
am11 wants to merge 6 commits into
dotnet:mainfrom
am11:chore/mop-strcpy

Conversation

@am11

@am11am11 commented Jul 24, 2026

Copy link
Copy Markdown
Member

Follow up #130547.

@am11am11 mentioned this pull request Jul 24, 2026
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jul 24, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 6 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

Comment threadsrc/coreclr/gc/unix/cgroup.cpp Outdated
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

@jkotasjkotasJul 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What guarantees that parent_directory_end has enough space? This pattern is likely to trigger static analyzers.

(not commenting on all places with this issue)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It's same guarantees as before; none.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The PR title is "Replace more strcpy with safe string copy". Why is this safe string copy then?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

strcpy -> memcpy/strcpy_s, because strcpy is being flagged by clang on OpenBSD warning: strcpy() is almost always misused, please use strlcpy().

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think Jan's point is that if we're changing this code at all, we should change it to something better, not to something that has the same problem and is somewhat harder to read than the original.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Not sure how it is harder to read than original given #130547 is merged?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was fine with #130547 since it was an open-coded strdup, no length arithmetic, length is computed on one line and used a few lines later in a very straightforward way. In hindsight, I should have suggested to just use strdup instead of open coding it. We use strdup in number of other places. Could you please replace it with strdup as part of these changes?

This code is not a straightforward like that.

We have SafeStringCopy in pal_utilities.h to solve this problem. Can we promote it or some variant of it to be usable everywhere? Would it be possible to standardize on strcpy_s and polyfill strcpy_s when it is not available - do we build on any platforms like that?

@am11
am11force-pushed the chore/mop-strcpy branch from 6a2d59d to c0f7062CompareJuly 24, 2026 11:29
@am11
am11force-pushed the chore/mop-strcpy branch from c0f7062 to 7ea1219CompareJuly 24, 2026 12:00
@am11
am11 requested a review from jkoritzinskyJuly 24, 2026 15:50
@am11

am11 commented Jul 24, 2026

Copy link
Copy Markdown
MemberAuthor

BA failure is #131256.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 continues the effort to eliminate strcpy usage across CoreCLR, NativeAOT, and related tools by replacing it with length-aware copies (primarily memcpy) and minor refactors to compute/carry explicit buffer lengths.

Changes:

  • Replaced multiple strcpy call sites with memcpy (often using strlen(...) + 1) across VM/JIT/GC/NativeAOT/host/tooling code.
  • Introduced explicit length variables in a few places to make copy sizes clearer and avoid repeated strlen in allocation/copy sequences.
  • Added a couple of small robustness tweaks (e.g., guarding malloc results before copying in corerun.cpp).

Reviewed changes

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

Show a summary per file
FileDescription
src/coreclr/vm/gdbjit.cppReplaces strcpy with memcpy in GDB JIT debug-info string materialization paths.
src/coreclr/utilcode/debug.cppReplaces strcpy with memcpy when building the assert expression+stacktrace display buffer.
src/coreclr/utilcode/check.cppUses memcpy for copying dynamically allocated CHECK failure messages.
src/coreclr/nativeaot/Runtime/unix/PalUnix.cppUses computed length + memcpy in PalCopyTCharAsChar.
src/coreclr/nativeaot/Runtime/unix/cgroupcpu.cppReworks cgroup path construction to use explicit lengths and memcpy instead of strcpy/strcat.
src/coreclr/nativeaot/Runtime/RhConfig.cppRefactors env-var name construction and switches embedded string copies to memcpy.
src/coreclr/nativeaot/Runtime/clrgc.enabled.cppReplaces strcpy concatenation with length-based memcpy copies.
src/coreclr/jit/fgdiagnostic.cppReplaces strcpy with memcpy in escape-substitution copying.
src/coreclr/interpreter/methodset.cppCopies config string via memcpy instead of strcpy.
src/coreclr/inc/outstring.hUpdates <string.h> include comment (no longer calls out strcpy).
src/coreclr/ildasm/dres.cppReplaces strcat/strcpy usage with manual memcpy for indentation/string building in resource dumping.
src/coreclr/ilasm/method.hppUpdates commented-out code to reference strcpy_s.
src/coreclr/hosts/corerun/corerun.cppUses length-based memcpy and guards malloc before copying core paths.
src/coreclr/gc/unix/cgroup.cppUses mount-length caching + memcpy for initial copy and replaces a strcpy with memcpy.

Comment threadsrc/coreclr/utilcode/debug.cpp Outdated
Comment threadsrc/coreclr/ildasm/dres.cpp Outdated
Comment threadsrc/coreclr/interpreter/methodset.cpp
am11and others added 2 commits August 13, 2026 10:07
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
cgroupPathLength = parent_directory_end - mem_limit_filename;

strcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME);
memcpy(parent_directory_end, CGROUP2_MEMORY_LIMIT_FILENAME, strlen(CGROUP2_MEMORY_LIMIT_FILENAME) + 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we gain anything here when the source is a string constant?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-VM-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@am11@jkotas@janvorli@MichalStrehovsky