Skip to content

Validate full path of file instead of file name in parsing /resource args#27279

Merged
OmarTawfik merged 3 commits intodotnet:masterfrom
OmarTawfik:bugs/27045/path-too-long
Jun 7, 2018
Merged

Validate full path of file instead of file name in parsing /resource args#27279
OmarTawfik merged 3 commits intodotnet:masterfrom
OmarTawfik:bugs/27045/path-too-long

Conversation

@OmarTawfik
Copy link
Copy Markdown
Contributor

Fixes #27045

The original bug is that the file name was checked instead of the full path. This makes the FileInfo constructor consider the execution path as the base path for the (relative) file name.

This change has no tests associated, as I talked with a couple of people on the team, and I believe this case is exteremely hard to put in a unit test, as the test needs to move the executed tests/compiler somewhere long enough to repro, and the scenario is corner case enough I don't think it is worth modifying the test harness at this point.

@OmarTawfik OmarTawfik added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 31, 2018
@OmarTawfik OmarTawfik added this to the 15.8 milestone May 31, 2018
@OmarTawfik OmarTawfik self-assigned this May 31, 2018
@OmarTawfik OmarTawfik requested a review from a team as a code owner May 31, 2018 01:41
Copy link
Copy Markdown
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

:shipit:

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented May 31, 2018

approved #Resolved

/// <param name="path"></param>
/// <returns></returns>
public static bool IsValidFilePath(string path)
public static bool IsValidFilePath(string fullPath)
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs May 31, 2018

Choose a reason for hiding this comment

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

public static bool IsValidFilePath(string fullPath) [](start = 8, length = 51)

Would it be valid to add an assert Debug.Assert(IsAbsolute(fullPath));? #Closed

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. Will add that.


In reply to: 192184509 [](ancestors = 192184509)

return null;
}

if (fullPath == null || !PathUtilities.IsValidFilePath(fileName))
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs May 31, 2018

Choose a reason for hiding this comment

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

fullPath == null [](start = 16, length = 16)

No need to check for null? #Closed

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.

IsValidFilePath does that check.


In reply to: 192184799 [](ancestors = 192184799)

Return Nothing
End If

If fullPath Is Nothing OrElse Not PathUtilities.IsValidFilePath(fileName) Then
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs May 31, 2018

Choose a reason for hiding this comment

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

fullPath Is Nothing OrElse [](start = 15, length = 27)

No need to check for null? #Closed

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.

IsValidFilePath does that check.


In reply to: 192185024 [](ancestors = 192185024)

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented May 31, 2018

Done with review pass (iteration 1) #Closed

@OmarTawfik OmarTawfik added Bug Area-Compilers and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels May 31, 2018
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 2)

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

@AlekseyTs commented out your suggestion because of an existing bug.
Added a note to bring back the original Debug.Assert() when it is resolved.

@OmarTawfik OmarTawfik merged commit e8a5117 into dotnet:master Jun 7, 2018
@OmarTawfik OmarTawfik deleted the bugs/27045/path-too-long branch June 7, 2018 20:58
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.

3 participants