Skip to content

Check whether ROOTSYS is defined in rootcling_stage1.#10776

Closed
chilikink wants to merge 4 commits into
root-project:masterfrom
chilikink:fix-std-string-null
Closed

Check whether ROOTSYS is defined in rootcling_stage1.#10776
chilikink wants to merge 4 commits into
root-project:masterfrom
chilikink:fix-std-string-null

Conversation

@chilikink
Copy link
Copy Markdown
Contributor

This Pull request:

Changes or fixes:

It is not checked in the existing code whether getenv("ROOTSYS") returns NULL. It does so if the environment variable is not defined; it is exactly what happens during building here. Then std::string gets initialized from the C string from address 0, which may result in a segmentation fault.

Checklist:

  • [ x ] tested changes locally
  • updated the docs (if necessary)

This PR fixes #

@phsft-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@Axel-Naumann
Copy link
Copy Markdown
Member

That's a symptom of another issue that we also need to understand. rootcling_stage1 is supposed to putenv ROOTSYS itself (in SetRootSys() in core/dictgen/src/rootcling_impl.cxx); it should not need to be set from the "outside". Do you know why SetRootSys() fails to do what it's supposed to?

@chilikink
Copy link
Copy Markdown
Contributor Author

Yes, this happened because I was trying to compile on an officially unsupported system. The function GetExePath() is not implemented and returns an empty string. It the new commit, an error message is printed in SetRootSys() in case of unknown executable path, and the missing implementation of GetExePath() is added. The program rootcling_stage1 now determines its ROOTSYS (build directory) successfully as confirmed by printing rootsys in GetIncludeDir(), although it then still fails reporting circular dependencies in the headers, but it is a separate issue.


static const char *GetIncludeDir() {
static std::string incdir = std::string(getenv("ROOTSYS")) + "/include";
const char *rootsys = getenv("ROOTSYS");
Copy link
Copy Markdown
Member

@Axel-Naumann Axel-Naumann Jun 17, 2022

Choose a reason for hiding this comment

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

Should we instead assert that ROOTSYS is set at this point? This is rootcling_stage1.cxx - ROOTSYS must have been set by rootcling itself.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree that it is a better idea since the variable should always be defined. As requested, the assertion is added together with a comment pointing to the place of definition.

@guitargeek
Copy link
Copy Markdown
Contributor

guitargeek commented Jan 9, 2024

Hi @chilikink, what's the status of this PR? Is this tweak still needed, and so you still want to get it merged? If yes, please let us know and resolve the conflicts by re-basing the commits on master.

@guitargeek
Copy link
Copy Markdown
Contributor

Superseded by #12996. Thanks for the initial effort, I'm sure it was helpful for the authors of the linked PR!

@guitargeek guitargeek closed this Sep 9, 2024
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.

5 participants