Fix undefined behavior in key_from_id()

[dovydenkovas]

key_from_id() checks for a resource extension but has errors when handling names without extension or when receiving NULL instead of string.

The declaration size_t extension_offset = -1; is incorrect, because size_t is an unsigned type. As a result, extension_offset >= 0 is always true. This may leads to:

• null pointer deference in bundled_resources_is_known_assembly_extension (extension);
• integer overflow and incorrect results in the expression extension_offset + 2.

Additionally, g_strlcpy requires a non-null string source string, so id == NULL causes UB in g_strlcpy(buffer, id, ...).

There are two ways to call key_from_id: in mono_bundled_resources_add(), which includes a check via g_assert (resource_to_bundle->id); and in bundled_resources_get(), which does not perform any checks on the id parameter.

To fix described problems change type of extension_offset from size_t to ssize_t to properly handle -1 initial value. This ensures extension_offset >= 0 accurately reflects whether a file extension was found, avoiding buffer overflow risk. Enforce id != NULL via assert in key_from_id and add explicit id != NULL check in bundled_resources_get.

Signed-off-by: Aleksandr Dovydenkov asd@altlinux.org
Found by Linux Verification Center (linuxtesting.org) with SVACE.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-resources
See info in area-owners.md if you want to be subscribed.

[dovydenkovas]

@dotnet-policy-service agree company="ALT Linux"

[steveisok]

@lewing I suspect this is useful for wasm.

[jeffhandley]

Note

This review was generated by the holistic code review workflow being iterated on as part of #130339. Please treat its findings as assistive input for human review.

Holistic Review

Motivation: The PR addresses a real native correctness issue in key_from_id: the unsigned size_t sentinel makes extension_offset >= 0 tautological, so resource IDs with no . can flow into strcmp with a null extension. This is relevant to Mono bundled resources, including WASM/WASI data resources.

Approach: Changing the sentinel to signed storage and guarding null lookup IDs is the right minimal fix. I checked the add/get callers and the bundle-generation side; the managed task emits RegisteredName strings and native code canonicalizes them consistently at both insertion and lookup.

Summary: ✅ LGTM. The fix preserves computed key/hash values for all previously well-defined IDs: known assembly extensions still canonicalize to the same .dll key, and non-known extensions still use the original ID. The only behavioral change I see is that no-extension IDs now use the original ID instead of hitting undefined behavior/null dereference, and null lookup now returns “not found.”

---

Detailed Findings

✅ Correctness — unsigned sentinel UB is addressed

In src/mono/mono/metadata/bundled-resources.c lines 91-113, the old size_t extension_offset = -1 made extension_offset >= 0 always true. When g_memrchr found no dot, extension stayed null and the old code could call bundled_resources_is_known_assembly_extension(extension), which immediately passed that null pointer to strcmp. The new ssize_t sentinel makes the no-extension path take the else branch and copy the original ID.

✅ Compatibility — canonical keys and hashes are preserved

For IDs with known assembly extensions (.dll, .webcil, and the configured webcil-in-wasm extension), extension_offset is still the same character offset, so the g_strlcpy(... extension_offset + 2) plus strcat("dll") sequence produces the same canonical .dll key as before. Since bundled_resources_resource_id_hash hashes the canonical key string, the hash value is unchanged whenever the old code had defined behavior. The managed bundle emitter in src/tasks/MonoTargetsTasks/EmitBundleTask/EmitBundleBase.cs and its templates only place the registered name into .resource.id; it does not compute an alternate key that would need updating.

✅ Null/no-extension callers — lookup behavior is safer

bundled_resources_get now returns NULL for null lookup IDs before calling key_from_id (src/mono/mono/metadata/bundled-resources.c lines 209-212). Add paths already assert resource IDs are non-null before insertion, and callers such as assembly.c, mono-debug.c, browser driver.c, and WASI driver.c pass concrete assembly/data names. WASI data lookups like icudt.dat and runtimeconfig.bin continue to use the original key because those extensions are not treated as assembly extensions.

💡 Tests/Cleanup — no dedicated regression test in this PR

I did not see test changes. Given the small native fix and the fact that the important compatibility property is “same key string for already-working IDs,” I do not consider that blocking, but a targeted bundled-resource lookup covering an ID without . would make the analyzer-found failure mode explicit if there is a convenient Mono/WASM test hook. The newly added block comment also appears more verbose than the surrounding style and contains trailing whitespace; consider trimming it in a follow-up or before merge if maintainers prefer.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "5096eab329e0a156ee8961cc9094af0e4e2d5374",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "13c03a4a9a854291f77636b1e15038e65919e68b",
  "last_reviewed_commit": "5096eab329e0a156ee8961cc9094af0e4e2d5374",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "13c03a4a9a854291f77636b1e15038e65919e68b",
  "last_recorded_worker_run_id": "29690795771",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "5096eab329e0a156ee8961cc9094af0e4e2d5374",
      "review_id": 4730930957
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Justified. key_from_id() declared size_t extension_offset = -1;, so the guard extension_offset >= 0 was always true, feeding a NULL extension into bundled_resources_is_known_assembly_extension() (which calls strcmp on it) and using a huge extension_offset + 2 in g_strlcpy. The id == NULL path also reached g_strlcpy(buffer, id, ...)/g_memrchr with a NULL source. These are real defects found by static analysis (SVACE).

Approach: Sound and minimal. Changing extension_offset to ssize_t makes the -1 sentinel and >= 0 check behave correctly; ssize_t is already used elsewhere in mono and is defined for MSVC in mono/utils/mono-compiler.h, so it is portable. Replacing the if (id) guard with g_assert (id) matches the existing precondition (mono_bundled_resources_add already asserts resource_to_bundle->id), and adding !id to the early-out in bundled_resources_get protects the other call site so the assert cannot fire in normal use.

Summary: ✅ LGTM. The three signed/NULL defects are correctly addressed, the change is portable and consistent with surrounding code, and the added doc comment is a nice touch. The only nit is trailing whitespace on a few newly added lines (78, 95, 100); no functional concerns.

---

Detailed Findings

✅ Correctness — signed extension offset

Switching to ssize_t fixes the always-true extension_offset >= 0 check, eliminating both the NULL-extension deref in bundled_resources_is_known_assembly_extension and the extension_offset + 2 overflow in g_strlcpy. The remaining size math (MIN(buffer_len - 4, extension_offset + 2)) is only reached when an extension was actually found, so extension_offset is a valid non-negative index.

✅ NULL handling — assert plus caller guard

The g_assert (id) documents the precondition, and the new !id check in bundled_resources_get covers the one caller that previously did not validate id. mono_bundled_resources_add already guarantees non-NULL ids, so the assert will not regress existing behavior.

💡 Style — trailing whitespace

The newly added lines 78 (// ), 95 (tab after g_assert (id);), and 100 (tab-only) contain trailing whitespace. Minor, but worth cleaning up (flagged inline on line 95).

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 72.7 AIC · ⌖ 10.4 AIC · ⊞ 10K

[github-actions]

💡 Trailing whitespace: this line ends with a stray tab after g_assert (id);. There is also trailing whitespace on the new comment line 78 (// ) and the tab-only line 100. Please strip these to keep the file clean and avoid tripping format/whitespace checks.