Skip to content

Profiler crashed in ElfParser - #793

Open
zhengyu123 wants to merge 9 commits into
mainfrom
zgu/elfparser
Open

zhengyu123 wants to merge 9 commits into
mainfrom
zgu/elfparser

Conversation

@zhengyu123

@zhengyu123 zhengyu123 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?:
Hardens ElfParser's parsing of the PT_DYNAMIC section and related virtual-address-relative structures (DT_HASH, DT_GNU_HASH, DT_SYMTAB/DT_STRTAB, .rela.plt/.rela.dyn relocation tables, the SFrame/eh_frame_hdr unwind sections) against corrupted or malformed ELF metadata found in a live-mapped shared library.

  • Adds ElfParser::inLiveImage() / liveSegmentEnd() / liveImageEnd(), a bounds-checking layer for pointers derived from at()/dyn_ptr() (virtual addresses in live process memory), parallel to the existing inImage() (which only bounds file-offset-relative pointers). Every such pointer is now validated against the ELF's own live PT_LOAD footprint before it is dereferenced.
  • Adds overflow-safe helpers resolveSymbol() and resolveImportAddr() used by both relocation loops, and hardens getSymbolCount()'s DT_GNU_HASH bucket/bloom/chain walk against attacker-controlled sizes and offsets (all arithmetic validated in a wide integer type before forming any pointer).
  • Clamps DT_STRSZ to the room actually left in strtab's own live segment, fixing string-table lookups that were previously either unbounded (trusting a corrupted DT_STRSZ) or over-strict (rejecting small, otherwise-valid libraries).
  • Caps the DT_GNU_HASH chain-terminator scan (MAX_GNU_HASH_CHAIN_SCAN, 1M entries) so a corrupted table that never sets its terminator bit can't turn into a very long scan of a large live segment while Symbols::_parse_lock is held.

Motivation:
Production crash:

ElfParser::parseDynamicSection()+0x158
ElfParser::parseProgramHeaders(CodeCache*, char const*, char const*, bool)+0xf8
Symbols::parseLibraries(CodeCacheArray*, bool)+0x4c0
Libraries::updateSymbols(bool)+0xc
Profiler::start(Arguments&, bool)+0x84c

The DT_HASH case in parseDynamicSection() dereferenced a dyn_ptr()-derived pointer with no bounds check — a single malformed DT_HASH entry crashed the process. This was the one path in symbols_linux.cpp not routed through any bounds check (everything else already used inImage() for file-offset-relative data). Rather than special-casing just DT_HASH, this fixes the underlying gap: every virtual-address-relative pointer computed via at()/dyn_ptr() in this file is now validated the same way, so the same class of bug can't resurface at DT_GNU_HASH, the relocation tables, or the SFrame/eh_frame_hdr sections.

Additional Notes:

  • MAX_PLAUSIBLE_SEGMENT_SIZE (4 GiB) caps how large a single PT_LOAD segment is trusted to be, guarding the pointer arithmetic in plausibleSegmentEnd()/liveSegmentEnd()/liveImageEnd() against wraparound from a corrupted p_memsz.
  • Ran this PR's own diff through a review pass; found and fixed two issues before merging:
    • The DT_GNU_HASH chain scan had no iteration cap, so a corrupted table landing in a large (now up to 4 GiB) live segment with its terminator bit never set could scan for a very long time, one inLiveImage() (O(e_phnum)) call at a time, while holding Symbols::_parse_lock. Added MAX_GNU_HASH_CHAIN_SCAN to bound this to a fast failure instead.
    • Removed a dead strtab_room == 0 branch in parseDynamicSection(): dyn_ptr() already guarantees strtab lies strictly inside a live segment before returning non-NULL, so that branch could never be taken.
    • at()/dyn_ptr() computed _vaddr_diff + p_vaddr (or + d_ptr) unconditionally; an untrusted p_vaddr/d_ptr large enough overflows that pointer addition (undefined behavior, and on wraparound could alias back into a plausible-looking live address). Both now validate the addition in integer space first and reject the entry (at() returns false, dyn_ptr() returns nullptr) instead of forming an overflowing pointer; all 5 call sites were updated to check the result.

How to test the change?:
ddprof-lib/src/test/cpp/elfparser_ut.cpp adds extensive coverage, including:

  • dynamicSectionHashPointerOutOfBounds: a regression test reproducing the exact production crash via a crafted DT_HASH pointer (must not crash, must not populate symbols).
  • Per-guard coverage isolating resolveSymbol()/resolveImportAddr() against a malformed syment, symbol index, or r_offset in .rela.plt entries.
  • atProgramHeaderVaddrOverflow / dynPtrOverflow: regression tests for the at()/dyn_ptr() overflow guard, using p_vaddr/d_ptr == UINT64_MAX to force the pointer addition to overflow.

Run on Linux: ./gradlew :ddprof-lib:gtestDebug_elfparser_ut (or gtestRelease_elfparser_ut).

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a security review (run the dd:platform-security-review
    skill, or file a request via the PSEC review form).
    bewaire also runs automatically on every PR.
  • This PR doesn't touch any of that.
  • JIRA: PROF-15982

Unsure? Have a question? Request a review!

@datadog-prod-us1-5

datadog-prod-us1-5 Bot commented Sep 15, 2026

Copy link
Copy Markdown

Pipelines  Tests

❌ Errors

Your PR has failed checks. Please review the issues below and take necessary action before merging.

🚦 4 Pipeline jobs failed

DataDog/java-profiler | gtest-asan-amd64

View more details · View in GitLab

DataDog/java-profiler | gtest-asan-arm64

View more details · View in GitLab

DataDog/java-profiler | gtest-tsan-amd64

View more details · View in GitLab

View all 4 failed jobs.

ℹ️ Info

🔄 Datadog auto-retried 1 job - 0 passed on retry View in Datadog

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: a33cab7 | Docs | View more details | Give us feedback!

@dd-octo-sts

dd-octo-sts Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Scan-Build Report

User:runner@runnervmlun5p
Working Directory:/home/runner/work/java-profiler/java-profiler/ddprof-lib/src/test/make
Command Line:make -j4 all
Clang Version:Ubuntu clang version 18.1.3 (1ubuntu1)
Date:Thu Sep 17 18:00:04 2026

Bug Summary

Bug TypeQuantityDisplay?
All Bugs1
Logic error
Dereference of null pointer1

Reports

Bug Group Bug Type ▾ File Function/Method Line Path Length
Logic errorDereference of null pointerfaultInjection.cppcrashNow242

@dd-octo-sts

dd-octo-sts Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

CI Test Results

Run: #35255933261 | Commit: 4bec60e | Duration: 15m 4s (longest job)

All 32 test jobs passed

Status Overview

JDK glibc-aarch64/debug glibc-amd64/debug musl-aarch64/debug musl-amd64/debug
8 - - -
8-ibm - - -
8-j9 - -
8-librca - -
8-orcl - - -
11 - - -
11-j9 - -
11-librca - -
17 - -
17-graal - -
17-j9 - -
17-librca - -
21 - -
21-graal - -
21-librca - -
25 - -
25-graal - -
25-librca - -

Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled

Summary: Total: 32 | Passed: 32 | Failed: 0


Updated: 2026-09-17 18:16:48 UTC

@zhengyu123 zhengyu123 added the sphinx:spotcheck Sphinx: spot-check recommended label Sep 15, 2026
@zhengyu123 zhengyu123 added sphinx:critical Sphinx: critical — human review required and removed sphinx:spotcheck Sphinx: spot-check recommended labels Sep 16, 2026
@zhengyu123
zhengyu123 marked this pull request as ready for review September 17, 2026 17:15
@zhengyu123
zhengyu123 requested a review from a team as a code owner September 17, 2026 17:15

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a5e34de3ac

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ddprof-lib/src/main/cpp/symbols_linux.cpp

@datadog-prod-us1-5 datadog-prod-us1-5 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Datadog Autotest: FAIL

A damaged secondary PT_LOAD address can overflow before the new bounds check runs. This fault can still crash the profiler.

Open Bits AI session

🤖 Datadog Autotest · Commit a5e34de · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest

Comment thread ddprof-lib/src/main/cpp/symbols_linux.cpp Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sphinx:critical Sphinx: critical — human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant