Skip to content

[core] Fix plugin lookup when Root.PluginPath has multiple directories - #23796

Closed
JainAyush-01 wants to merge 1 commit into
root-project:masterfrom
JainAyush-01:fix/23536-root-plugin-path
Closed

JainAyush-01 wants to merge 1 commit into
root-project:masterfrom
JainAyush-01:fix/23536-root-plugin-path

Conversation

@JainAyush-01

@JainAyush-01 JainAyush-01 commented Oct 22, 2026 •

Copy link
Copy Markdown

This Pull request:

Fixes an in-place string mutation bug in TPluginManager::LoadHandlersFromPluginDirs that breaks plugin discovery when Root.PluginPath (or ROOT_PLUGIN_PATH) contains multiple colon-separated directories.

Changes or fixes:

Root Cause:

In commit e60640c (#18859), gSystem->ConcatFileName was replaced with gSystem->PrependPathName. Unlike ConcatFileName which returned a new heap-allocated string without modifying the input, PrependPathName(const char *dir, TString &name) modifies name in place.

In TPluginManager::LoadHandlersFromPluginDirs(const char *base):

TString sbase = base;
...
for (Int_t i = 0; i < dirs->GetEntriesFast(); i++) {
   ...
   if (!skip) {
      if (sbase != "") {
         const char *p = gSystem->PrependPathName(d, sbase);
         LoadHandlerMacros(p);
      }
   }
}

Because sbase was passed directly to PrependPathName without copying to a temporary variable:

  • On iteration 0 (dir0 = "/custom/plugins"): sbase was mutated from "TArchiveFile" to "/custom/plugins/TArchiveFile".
  • On iteration 1 (dir1 = "$ROOTSYS/etc/plugins"): PrependPathName prepended dir1 to the already mutated sbase, resulting in "$ROOTSYS/etc/plugins//custom/plugins/TArchiveFile".

As a result, whenever a user or experiment environment (e.g. ALICE O2 / JAliEn) prepended a custom directory to Root.PluginPath, all subsequent directories—including ROOT's own built-in plugins in $ROOTSYS/etc/plugins—failed to load.

Fix:

Copy sbase to a temporary TString temp = sbase; inside the loop before calling PrependPathName(d, temp), matching the pattern already used in the else branch (lines 545–546) and throughout TPluginManager.cxx.

Testing:

  • Added GoogleTest regression test TEST(TPluginManager, MultiDirectoryPluginPath) in core/base/test/TPluginManagerTests.cxx.
  • Verified that both custom plugins in a prepended directory and built-in plugins in subsequent directories are resolved properly.
  • Ran test via ctest -R CorePluginManagerTests: 100% passed (0.26s).

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #23536


AI-assisted coding disclosure: I used an AI assistant to help navigate the codebase, structure the unit test, and format this PR description. I have personally reviewed, debugged, and tested the C++ changes locally and take full responsibility for them.

When Root.PluginPath (or ROOT_PLUGIN_PATH) contains multiple colon-separated
directories, TPluginManager::LoadHandlersFromPluginDirs loops over each
directory to load handler macros for the requested base class.

In commit e60640c (root-project#18859), ConcatFileName was replaced by PrependPathName.
Unlike ConcatFileName which returned a new heap-allocated string,
PrependPathName modifies its second parameter in-place. Because `sbase`
was passed directly:
    const char *p = gSystem->PrependPathName(d, sbase);
the variable `sbase` was mutated in-place across iterations (e.g., from
"TArchiveFile" to "/dir1/TArchiveFile", then to "/dir2//dir1/TArchiveFile").
Consequently, subsequent search directories received a mangled path and
ROOT could not find its own built-in plugins whenever a custom plugin
directory was prepended.

Fix this by copying `sbase` to a temporary `TString temp = sbase;` on each
iteration, matching the pattern used in the rest of TPluginManager.cxx.

Fixes root-project#23536. Added regression test in core/base/test/TPluginManagerTests.cxx.
@dpiparo

dpiparo commented Oct 25, 2026

Copy link
Copy Markdown
Member

@JainAyush-01

Copy link
Copy Markdown
Author

Hi @dpiparo,

Thank you for pointing this out. I apologize, I missed the AI disclosure section in CONTRIBUTING.md.

I have updated the description here (and on #23791) with the disclosure. I used an LLM to help explore the codebase and structure the test, but I made sure to verify the bug and C++ logic locally, sbase was passed by non const reference TString and inside the vpaths loop, so sbase.Prepend(...) was repeatedly accumulating previous path segments instead of evaluating each search path independently.

Going forward, I will make sure all tool usage is properly disclosed according to the guidelines

@pcanal

pcanal commented Oct 30, 2026

Copy link
Copy Markdown
Member

Thanks for you contribution. The test has been fixed and the PR has been extended as part of #23928

@pcanal pcanal closed this Oct 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ROOT_PLUGIN_PATH is handled incorrectly

3 participants