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.

Sign up for GitHub

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

Jump to bottom

Fix remote server path resolution #4131

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Fix remote server path resolution #4131

merged 1 commit into from
Dec 14, 2023

Conversation

vodorok
Copy link
Contributor

@vodorok vodorok commented Dec 13, 2023

No description provided.

@vodorok vodorok requested a review from bruntib as a code owner December 13, 2023 12:39
whisperity
whisperity reviewed Dec 13, 2023
View reviewed changes
codechecker_common/review_status_handler.py Outdated
Comment on lines 322 to 325
source_file_name = os.path.realpath(os.path.join(
self.__source_root, report.file.original_path))
self.__source_root, report.file.original_path.strip('/')))
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Should this use lstrip instead of plain strip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Valid question, the goal is to make report.file.original_path not absolute. lstrip would only remove the forward slash from the front of the path string.

codechecker_common/review_status_handler.py Outdated
Comment on lines 322 to 325
source_file_name = os.path.realpath(os.path.join(
self.__source_root, report.file.original_path))
self.__source_root, report.file.original_path.strip('/')))
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Why isn't this using the fakeroot_path from 7a98601?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I need to better understand if path manipulation can be achieved at this stage.
Eg.: can Mallory construct an original_path that can navigate out of the temp folder, and cause harm there.

@whisperity whisperity added CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands bugfix 🔨 labels Dec 13, 2023
@whisperity
Copy link
Contributor

Another question: Shouldn't this patch first go into the release-v6.23.0 branch (which does not exist yet) and then be cherry-picked onto master? Or is going into master first and then getting backported to the fix-branch the better way?

@vodorok
Copy link
Contributor Author

vodorok commented Dec 13, 2023 •
edited
Loading

@whisperity We are planning to release a patch release today (or tomorrow), as it affects all servers that is not running in a docker container.

@vodorok vodorok force-pushed the remote_path_fix branch 10 times, most recently from f0b88aa to 29ae5b6 Compare December 13, 2023 19:03
@vodorok vodorok requested a review from dkrupp December 13, 2023 19:04
@vodorok vodorok force-pushed the remote_path_fix branch 2 times, most recently from 46c68d6 to 1ac6077 Compare December 13, 2023 19:55
Szelethus
Szelethus reviewed Dec 13, 2023
View reviewed changes
codechecker_common/review_status_handler.py Outdated
report.file.original_path, self.__source_root)
else:
# in this branch we are operating on the original filesystem,
# and we need the path to he original file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# and we need the path to he original file
# and we need the path to the original file

Copy link
Contributor

Choose a reason for hiding this comment

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

"On this branch the server runs on the same file system where the source file is"

Copy link
Contributor

Choose a reason for hiding this comment

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

Its very interesting and not at all trivial why we need to branch on this -- can you create an issue to remind us to document this properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, for the review, will make an issue tomorrow

@bruntib bruntib merged commit a1a2df7 into Ericsson:master Dec 14, 2023
8 checks passed
@whisperity whisperity added this to the release 6.23.1 milestone Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whisperity whisperity whisperity left review comments

@Szelethus Szelethus Szelethus approved these changes

@bruntib bruntib bruntib approved these changes

@dkrupp dkrupp Awaiting requested review from dkrupp

Assignees
No one assigned
Labels
bugfix 🔨 CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands
Projects
None yet
Milestone
release 6.23.1
Development

Successfully merging this pull request may close these issues.
4 participants
@vodorok @whisperity @bruntib @Szelethus