Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TClingUtils: Avoid growing paths in GetFileName #10387

Merged
merged 1 commit into from
Apr 21, 2022

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Apr 12, 2022

Strip of ../ so that the search finds a valid longest match. With the upcoming upgrade of LLVM 13, this would otherwise lead to an error "File name too long" but it's already wrong now.

@phsft-bot

This comment was marked as outdated.

@hahnjo hahnjo self-assigned this Apr 12, 2022
@phsft-bot

This comment was marked as resolved.

@phsft-bot

This comment was marked as resolved.

@phsft-bot

This comment was marked as resolved.

@phsft-bot

This comment was marked as resolved.

@phsft-bot

This comment was marked as resolved.

@phsft-bot

This comment was marked as resolved.

llvm::SmallString<256> headerFileName(headerFE->getName());
// Remove double ../ from the path so that the search below finds a valid
// longest match and does not result in growing paths.
llvm::sys::path::remove_dots(headerFileName, /*remove_dot_dot=*/true);
Copy link
Member

Choose a reason for hiding this comment

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

IIUC this will break line 3402 in the original file:

      size_t lenTrailing = headerFileName.size() - (IDir->data() - headerFileName.data());

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, IDir is also created referencing headerFileName. That should be fine. The use-after-free is definitely not 😐

Copy link
Member

Choose a reason for hiding this comment

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

Let’s count on a post review here if that’s still unclear

@hahnjo
Copy link
Member Author

hahnjo commented Apr 12, 2022

Um yeah, that's a nice use-after-free right there because TMetaUtils::GetFileName returns trailingPart which is a StringRef... I have to think how to fix

@hahnjo hahnjo marked this pull request as draft April 12, 2022 13:55
Strip of ../ so that the search finds a valid longest match. With the
upcoming upgrade of LLVM 13, this would otherwise lead to an error
"File name too long" but it's already wrong now.
@hahnjo hahnjo marked this pull request as ready for review April 12, 2022 14:15
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1015/python3, mac11/cxx17, windows10/cxx14
How to customize builds

@hahnjo hahnjo mentioned this pull request Apr 13, 2022
8 tasks
@hahnjo
Copy link
Member Author

hahnjo commented Apr 20, 2022

ping @vgvassilev

Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM!

llvm::SmallString<256> headerFileName(headerFE->getName());
// Remove double ../ from the path so that the search below finds a valid
// longest match and does not result in growing paths.
llvm::sys::path::remove_dots(headerFileName, /*remove_dot_dot=*/true);
Copy link
Member

Choose a reason for hiding this comment

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

Let’s count on a post review here if that’s still unclear

@hahnjo hahnjo merged commit cdfa621 into root-project:master Apr 21, 2022
@hahnjo hahnjo deleted the tcling-dot-dot branch April 21, 2022 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants