Skip to content

Fixes for spindle's internal path parsing#148

Open
mplegendre wants to merge 3 commits intollnl:develfrom
mplegendre:bugfix/rocprof-crash
Open

Fixes for spindle's internal path parsing#148
mplegendre wants to merge 3 commits intollnl:develfrom
mplegendre:bugfix/rocprof-crash

Conversation

@mplegendre
Copy link
Member

This fixes two bugs, both related to the spindle server's path parsing:

First, files in the root filesystem were being mis-parsed by internal spindle path parsing. Something like "/bin" would get parsed into a file and directory component, and we were incorrectly making the directory the empty string in this case. That would fool other parts of spindle into thinking it was a relative path, and thus "/bin" would get cached by spindle as "${PWD}/bin".

While fixing that I added additional tests around path parsing and discovered more issues around symlinks and paths that use "..". In short, we were resolving the ".." in those paths before resolving symlinks. That's not wise, as a ".." in a path moves up a directory from a symlink target, not source.

I significantly rewrote path parsing for a couple places in spindle where this matters, and added new tests.

@mplegendre
Copy link
Member Author

@rountree -- Ping. Can you review this?

@nchaimov
Copy link
Collaborator

I've looked at the path parsing code and done some testing with the parseFilenameNoAlloc2 function. I found that if we give parseFilenameNoAlloc2 a path like "/bin/..", then we end up with dir = "/bin" and file = "..", even though in this case the whole path refers to a directory, not a file. I know that it's intentionally not handling .. components, but is it acceptable for ".." to end up as the file?

The static TLS commit is labeled as a WIP — was that meant to be included in this PR?

@mplegendre
Copy link
Member Author

Good catch on the "/bin/..". That's an annoying case, and I'll have to think about what we do there.
The WIP commit slipped in accidently. Thanks for catching that too.

Will fix.

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