Skip to content

fix Utilities.ResolvePath for relative paths#36

Merged
LordMike merged 3 commits into
DiscUtils:masterfrom
ahdde:relativepath
Jul 17, 2017
Merged

fix Utilities.ResolvePath for relative paths#36
LordMike merged 3 commits into
DiscUtils:masterfrom
ahdde:relativepath

Conversation

@zivillian

Copy link
Copy Markdown
Contributor

Fixes an error with resolving relative symlinks pointing to a target inside the same directory.

Example:

user@host:/etc$ ls -la
insgesamt 12
drwxr-xr-x  3 root  root 4096 Jul  8 15:03 .
drwxrwxrwt 14 root  root 4096 Jul  8 15:03 ..
drwxr-xr-x  2 root  root 4096 Jul  8 15:03 init.d
lrwxrwxrwx  1 root  root    6 Jul  8 15:03 rc.d -> init.d

@zivillian

Copy link
Copy Markdown
Contributor Author

There was an additional error with Path.GetFullPath which produces a path starting with the current working dir, if the basePath does not start with '\'.
At first I tried to replace the Trim() with TrimEnd() in DiscFileSystemInfo but this introduced failing tests for Iso9660.

@LordMike LordMike left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've just realized the entire path-manipulating-layer might be riddled with bugs.. :/


if (!basePath.EndsWith(@"\"))
basePath = Path.GetDirectoryName(basePath);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we can do this. On Linux and Windows, Path.* might give different results. An example is GetFileName.

On Windows, for a Windows path, it will return a file name as expected. But on linux, a Windows path will give you the whole path, and not just the name. The reason is the directory seperators which for Windows contain both variants ("" and "/") - on linux, only "/" is present.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can see other parts of this code works on the ""-premise, but for new code we need to be careful.

if (path.Length > 0 && path[0] != '\\')
{
path = '\\' + path;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In principle we have Path.IsRooted - but we need to consider OS-specifics here.

<GenerateAssemblyProductAttribute>false</GenerateAssemblyProductAttribute>
<PublicSign>true</PublicSign>
<SignAssembly>True</SignAssembly>
<AssemblyOriginatorKeyFile>../../SigningKey.snk</AssemblyOriginatorKeyFile>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Huh... Didn't realize tests weren't signed.. Oh well :P

CheckResolvePath(@"\etc\rc.d\", @"init.d", @"\etc\rc.d\init.d");
// For example: (\TEMP\Foo.txt, ..\..\Bar.txt) gives (\Bar.txt).
CheckResolvePath(@"\TEMP\Foo.txt", @"..\..\Bar.txt", @"\Bar.txt");
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we have tests anywhere for paths going outside the current volume?

@zivillian

Copy link
Copy Markdown
Contributor Author

I found this problem while dumping a whole fs tree to validate my btrfs implementation. To be sure it's actually a bug, I reproduced this on xfs and ext.
I share your fear for bugs in all Path related code, but I guess the best way is to create an issue outside of this PR to plan the refactoring and define the consistent test cases for some kind of System.IO.Path replacement.

@LordMike LordMike merged commit 52edde0 into DiscUtils:master Jul 17, 2017
@zivillian zivillian deleted the relativepath branch July 17, 2017 18:40
LordMike added a commit that referenced this pull request Aug 16, 2017
Includes PRs:
#17, #18, #21, #22, #23, #27, #30, #31, #33, #34, #35, #36, #38, #39, #40, #41, #48, #51, #52, #55, #60
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