Skip to content

feat(pj_base,pj_plugins): read plugin manifests without loading the DSO - #163

Open
pabloinigoblasco wants to merge 2 commits into
mainfrom
feat/pj-plugin-descriptor-section
Open

feat(pj_base,pj_plugins): read plugin manifests without loading the DSO#163
pabloinigoblasco wants to merge 2 commits into
mainfrom
feat/pj-plugin-descriptor-section

Conversation

@pabloinigoblasco

Copy link
Copy Markdown
Collaborator

feat(pj_base,pj_plugins): read plugin manifests without loading the DSO

Summary

  • New descriptor section. Every DSO built with a PJ_*_PLUGIN macro
    now emits a self-describing blob — magic, ABI version, family, manifest
    JSON — into a dedicated section: .pj_manifest (ELF), .pjmani (PE),
    __PJ,__manifest (Mach-O). It is located by section, not by symbol,
    so it survives stripping. See
    pj_base/include/pj_base/plugin_descriptor_section.hpp.
  • New object-file reader
    (pj_plugins/src/detail/descriptor_section_reader.{hpp,cpp}) for
    ELF32/64 (both endiannesses), PE and thin Mach-O. Fields are read at
    fixed offsets rather than through <elf.h> / <windows.h> /
    <mach-o/loader.h>, so all three parsers compile and are testable on
    any host. Only the container headers and the section itself are read:
    inspecting a 50 MB plugin costs a few KB and ~2 ms.
  • inspectPluginDso prefers the static path and only falls back to
    dlopen + vtable probe + dlclose for a DSO with no descriptor section.
  • An ABI mismatch is now rejected from the section alone, so a plugin
    built against a different ABI is no longer loaded in order to discover
    that it should not have been.

Why

Discovery obtained the manifest by dlopen'ing each candidate, calling
its vtable entry point, and dlclose'ing. That leaves the plugin
resident. glibc pins a library NODELETE as soon as it defines an
STB_GNU_UNIQUE symbol, which any vague-linkage static produces
(inline-function locals, template statics, Meyers singletons). When a
second copy of the same plugin is later loaded from a different path,
it binds its own references to the first copy's storage and finds the
initialisation guards already set — so its own constructors never run
and any layout drift between the two builds corrupts the process.

This is not theoretical: all 22 bundled plugins export UNIQUE symbols
(one representative plugin exports 1117 of them, another 358). Even
first-party plugins built with pj_emit_plugin_manifest and its
-fvisibility=hidden export some, because libstdc++ declares
namespace std with explicit default visibility, so __gen_vtable /
__to_chars_10_impl instantiations reach .dynsym anyway.

Two reproducible startup crashes, both fixed here:

Scenario Crash site before
A locally built copy loaded via --plugin-dir over a bundled plugin PluginRuntimeCatalog::collectDeduplicatedPlugins
A marketplace copy newer than the bundled one — no CLI flag involved ExtensionCatalogService::seedBundledPlugins

Notes

  • Vtable-shape validation moves out of discovery for plugins taking
    the static path (protocol_version, struct_size, required slots).
    The contract is not loosened: the family loaders
    (data_source_library.cpp and siblings) already validate every plugin
    at load time. The two deliberately-broken test plugins hand-write
    their vtables, so they have no descriptor section, take the fallback,
    and their diagnostics are unchanged.
  • Plugins must be rebuilt to benefit. A DSO built against an older
    SDK has no section and still takes the dlopen path — including
    prebuilt third-party plugins that stay exposed until they are rebuilt
    against this SDK.
  • PJ_DIALOG_PLUGIN's body moved into PJ_DIALOG_PLUGIN_IMPL. Only
    the form taking a manifest emits a blob; the manifest-less form still
    routes to the static-link getter under PJ_STATIC_PLUGINS.
  • Each plugin gains one exported symbol per family
    (pj_plugin_descriptor_*). With -DPJ_ENABLE_ABI_CHECK=ON, abidiff
    reports it as an addition (libabigail bit 4, warning), not an
    incompatible change. abi/baseline.abi is untouched.

Testing

  • ./build.sh --debug && ./test.sh — full suite under ASAN.
  • ctest -R descriptor_section_reader_test — 18 cases over synthetic
    ELF64/ELF32/big-endian ELF/PE/Mach-O containers, including several
    blobs separated by linker padding, a section name sharing the prefix,
    PE file-alignment padding, an unknown blob version, and a manifest
    length running past the section.
  • ctest -R plugin_catalog_test — in particular
    InspectingADsoDoesNotLeaveItMapped, which fails if anyone
    reintroduces a dlopen into discovery, and
    MultiFamilyDsoReportsTheHighestPrecedenceFamily.
  • End-to-end against a real host build: place two divergent builds of
    one plugin id in the bundled dir and in a --plugin-dir override,
    then repeat with the second copy in the marketplace dir instead. Both
    used to SIGSEGV at startup. Expect no crash, and exactly one plugin
    load — the winning copy — instead of one per path.
  • Confirm the fallback still discovers plugins built against the
    previous SDK (they report zero .pj_manifest sections under
    readelf -S).
  • Worth checking on Windows: the PE path is covered by unit tests but
    has not run against a real .dll.

## Summary

- **New descriptor section.** Every DSO built with a `PJ_*_PLUGIN` macro
  now emits a self-describing blob — magic, ABI version, family, manifest
  JSON — into a dedicated section: `.pj_manifest` (ELF), `.pjmani` (PE),
  `__PJ,__manifest` (Mach-O). It is located by section, not by symbol,
  so it survives stripping. See
  `pj_base/include/pj_base/plugin_descriptor_section.hpp`.
- **New object-file reader**
  (`pj_plugins/src/detail/descriptor_section_reader.{hpp,cpp}`) for
  ELF32/64 (both endiannesses), PE and thin Mach-O. Fields are read at
  fixed offsets rather than through `<elf.h>` / `<windows.h>` /
  `<mach-o/loader.h>`, so all three parsers compile and are testable on
  any host. Only the container headers and the section itself are read:
  inspecting a 50 MB plugin costs a few KB and ~2 ms.
- **`inspectPluginDso` prefers the static path** and only falls back to
  `dlopen` + vtable probe + `dlclose` for a DSO with no descriptor section.
- **An ABI mismatch is now rejected from the section alone**, so a plugin
  built against a different ABI is no longer loaded in order to discover
  that it should not have been.

## Why

Discovery obtained the manifest by `dlopen`'ing each candidate, calling
its vtable entry point, and `dlclose`'ing. That leaves the plugin
resident. glibc pins a library `NODELETE` as soon as it defines an
`STB_GNU_UNIQUE` symbol, which any vague-linkage static produces
(inline-function locals, template statics, Meyers singletons). When a
second copy of the same plugin is later loaded from a different path,
it binds its own references to the first copy's storage and finds the
initialisation guards already set — so its own constructors never run
and any layout drift between the two builds corrupts the process.

This is not theoretical: all 22 bundled plugins export `UNIQUE` symbols
(one representative plugin exports 1117 of them, another 358). Even
first-party plugins built with `pj_emit_plugin_manifest` and its
`-fvisibility=hidden` export some, because libstdc++ declares
`namespace std` with explicit default visibility, so `__gen_vtable` /
`__to_chars_10_impl` instantiations reach `.dynsym` anyway.

Two reproducible startup crashes, both fixed here:

| Scenario | Crash site before |
|---|---|
| A locally built copy loaded via `--plugin-dir` over a bundled plugin | `PluginRuntimeCatalog::collectDeduplicatedPlugins` |
| A marketplace copy newer than the bundled one — no CLI flag involved | `ExtensionCatalogService::seedBundledPlugins` |

## Notes

- **Vtable-shape validation moves out of discovery** for plugins taking
  the static path (`protocol_version`, `struct_size`, required slots).
  The contract is not loosened: the family loaders
  (`data_source_library.cpp` and siblings) already validate every plugin
  at load time. The two deliberately-broken test plugins hand-write
  their vtables, so they have no descriptor section, take the fallback,
  and their diagnostics are unchanged.
- **Plugins must be rebuilt to benefit.** A DSO built against an older
  SDK has no section and still takes the `dlopen` path — including
  prebuilt third-party plugins that stay exposed until they are rebuilt
  against this SDK.
- **`PJ_DIALOG_PLUGIN`'s body moved into `PJ_DIALOG_PLUGIN_IMPL`.** Only
  the form taking a manifest emits a blob; the manifest-less form still
  routes to the static-link getter under `PJ_STATIC_PLUGINS`.
- Each plugin gains one exported symbol per family
  (`pj_plugin_descriptor_*`). With `-DPJ_ENABLE_ABI_CHECK=ON`, `abidiff`
  reports it as an addition (libabigail bit 4, warning), not an
  incompatible change. `abi/baseline.abi` is untouched.

## Testing

- `./build.sh --debug && ./test.sh` — full suite under ASAN.
- `ctest -R descriptor_section_reader_test` — 18 cases over synthetic
  ELF64/ELF32/big-endian ELF/PE/Mach-O containers, including several
  blobs separated by linker padding, a section name sharing the prefix,
  PE file-alignment padding, an unknown blob version, and a manifest
  length running past the section.
- `ctest -R plugin_catalog_test` — in particular
  `InspectingADsoDoesNotLeaveItMapped`, which fails if anyone
  reintroduces a `dlopen` into discovery, and
  `MultiFamilyDsoReportsTheHighestPrecedenceFamily`.
- End-to-end against a real host build: place two divergent builds of
  one plugin id in the bundled dir and in a `--plugin-dir` override,
  then repeat with the second copy in the marketplace dir instead. Both
  used to SIGSEGV at startup. Expect no crash, and exactly one plugin
  load — the winning copy — instead of one per path.
- Confirm the fallback still discovers plugins built against the
  previous SDK (they report zero `.pj_manifest` sections under
  `readelf -S`).
- Worth checking on Windows: the PE path is covered by unit tests but
  has not run against a real `.dll`.
@facontidavide

Copy link
Copy Markdown
Contributor

Cross-checked this PR against two independent investigations of the duplicate-mapping bug (one of them ran live dlopen experiments on the two mosaico copies). The design holds up — the section-located blob, the MSVC dllexport keep-alive, SHF_GNU_RETAIN, constinit, and the multi-family precedence matching the dlopen probe order all check out. Three small suggestions before merge, plus one observation worth recording.

1. Fix the isolation comments this PR's own rationale disproves

The empirical findings contradict two existing comments that are one directory away from the new header:

  • pj_plugins/src/detail/library_loader.hpp (the RTLD_LOCAL rationale): it claims each plugin "resolves its own copies of bundled statics in isolation from other plugins". For STB_GNU_UNIQUE symbols this is exactly wrong — GNU-unique exists to defeat RTLD_LOCAL. Verified live: a representative gRPC unique symbol resolved to the same address through two separate RTLD_LOCAL handles of the two mosaico builds, and loader diagnostics showed the second copy binding into the first copy's storage (its initialisation guards already set, so its constructors never ran — the duplicate gRPC metric-registration warnings reproduce this).
  • cmake/PjPluginManifest.cmake (the symbol-isolation block): -Wl,-Bsymbolic-functions cannot cover this case either — unique symbols are data objects, not functions.

Both comments should carry the GNU-unique caveat so the next reader doesn't trust the isolation claim. Suggest doing it in this PR since the new header is now the canonical description of the mechanism and can be cross-referenced.

Related precision fix in plugin_descriptor_section.hpp itself: "glibc pins a DSO NODELETE as soon as it defines an STB_GNU_UNIQUE symbol" is slightly overbroad. Pinning happens when the DSO is the first provider of a unique name entered into the namespace's unique table (do_lookup_unique in glibc's dl-lookup.c); a later copy that introduces no new names is not pinned and can even unload. Measured on the current 14-plugin AppDir: all 14 export unique symbols, but a sequential probe pinned only 4 — the rest found their names already provided. One-word fix ("first defines"/"first provides"), but it's a load-bearing comment.

2. Consider a load-time section-vs-vtable cross-check

Dedup, seeding, and compatibility decisions now run on the section manifest, while the vtable manifest "stays authoritative once the plugin is genuinely loaded" — and nothing verifies the two agree. A build-system glitch that desyncs them (stale blob after a manifest edit, packaging mixup) would silently drive winner selection with metadata the loaded plugin doesn't actually report.

Suggestion: when a winner is loaded, compare the vtable manifest's id/version/family against the section-derived descriptor and emit a warning diagnostic on mismatch (no behaviour change). This is not a security control — a malicious DSO can lie consistently in both places — it's an integrity check for honest builds, and it's cheap because the loaders already read the vtable manifest.

3. Retire the PE caveat with a real-DLL CI assertion

The PR notes the PE path "has not run against a real .dll". The test suite already builds mock plugin DSOs — running the plugin_catalog_test static-path assertions (in particular InspectingADsoDoesNotLeaveItMapped / the "section path taken, no dlopen" property) on the Windows lane against a real built DLL would close that gap before merge instead of after, and would also exercise the #pragma section + __declspec(allocate) emission end-to-end.

Observation (no change needed) + a caveat for the follow-up hardening

The ELF visibility("default") on pj_plugin_descriptor_* looks redundant next to used, retain, but it's a useful redundancy: an exported symbol is a gc-root, so it protects the section under --gc-sections on toolchains too old for SHF_GNU_RETAIN — the same job dllexport does on MSVC. Worth one extra line in the PJ_PLUGIN_DESCRIPTOR_KEEP comment (it currently explains used and retain but not why default visibility).

The flip side lands in the plugin repos later: the natural follow-up hardening (--exclude-libs,ALL / version scripts plus a zero-STB_GNU_UNIQUE-exports release gate on official plugins) must allowlist pj_plugin_descriptor_* alongside pj_plugin_abi_version and the family/dialog entry points — otherwise it strips the very keep-alive this PR relies on.

Out of scope here but queued as follow-ups: legacy section-less DSOs still take the dlopen fallback (and an old-ABI plugin is still dlopened — and potentially pinned — just to be rejected); containment for that remainder would be subprocess-based inspection. App side: submodule bump + rebuilding the bundled plugins (the fix is inert for any DSO not rebuilt against this SDK), and the AppImage packaging whitelist currently drops the .pjmanifest.json sidecars this repo generates.

🤖 Generated with Claude Code

…nly read

Review-driven follow-ups on the descriptor-section change:

- Comment precision on the STB_GNU_UNIQUE mechanism. RTLD_LOCAL,
  -fvisibility=hidden and -Wl,-Bsymbolic-functions all fail to contain
  unique data symbols by construction; the rationale in library_loader.hpp,
  PjPluginManifest.cmake, and plugin_descriptor_section.hpp now names the
  first-provider rule (do_lookup_unique in glibc's dl-lookup.c) explicitly
  and documents which category of symbols each mechanism does and does
  not cover.
- visibility("default") on the descriptor blob is now documented as a
  deliberate gc-root backup for toolchains without SHF_GNU_RETAIN, not
  as a redundant belt-and-braces.
- Expose readSectionDescriptor() as a public primitive next to
  inspectPluginDso(). It returns the section-derived PluginDescriptor
  without ever mapping the DSO, so callers that already hold a
  vtable-derived descriptor from a live plugin can diff the two and
  surface stale-blob or packaging-mixup mismatches as diagnostics. This
  is not a security control: a malicious DSO can lie consistently in
  both places.
- Test coverage for the new primitive against the mock data-source
  plugin.
@pabloinigoblasco

Copy link
Copy Markdown
Collaborator Author

Thanks for the careful read — pushed a follow-up commit addressing the three actionable points.

Precision on the first-provider mechanism. The rationale in library_loader.hpp, PjPluginManifest.cmake, and plugin_descriptor_section.hpp now says explicitly that pinning happens when the DSO is the first provider of a name entered into the namespace's process-wide unique table (do_lookup_unique in glibc's dl-lookup.c), and that later copies with no new names bind INTO the first copy's storage rather than being pinned themselves. That matters — it's the piece that explains why the duplicate-mapping bug is order-sensitive.

Scope caveat on symbol-isolation flags. RTLD_LOCAL, -fvisibility=hidden and -Wl,-Bsymbolic-functions all fail to contain STB_GNU_UNIQUE data by construction, and the comments now say which category each mechanism does and does not cover. In particular -Bsymbolic-functions is a function-call rewrite that does not touch data bindings; the follow-up hardening path (linker version scripts or -fno-gnu-unique) is referenced as what closes the gap this file cannot.

Section-only read exposed. readSectionDescriptor() is now a public primitive next to inspectPluginDso(). It returns the section-derived PluginDescriptor without ever mapping the DSO, so a caller that already holds a live plugin's vtable-derived descriptor can diff the two and surface stale-blob or packaging mixups as diagnostics. Header comment states plainly that this is diagnostic-only, not a security control — a malicious DSO can lie consistently in both places. Test coverage added.

visibility("default") on the descriptor blob is also documented now as the deliberate gc-root backup for toolchains without SHF_GNU_RETAIN, not redundant belt-and-braces.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants