Produce errors on invalid pdbpath supplied to compiler by OmarTawfik · Pull Request #25903 · dotnet/roslyn

OmarTawfik · 2018-04-03T00:49:56Z

Customer scenario

In case a path was supplied to the compiler with an invalid character, we use that path without reporting errors and stamp it into the embedded pdb within the assembly. We should detect that and report the appropriate error.

Bugs this fixes

Fixes #23525

Workarounds, if any

None.

Risk

This is a breaking change, where instead of allowing invalid paths, we provide appropriate errors.
@dotnet/roslyn-compat-council will be notified and consulted before merging.

Performance impact

Negligible.

Is this a regression from a previous update?

No

Root cause analysis

Validation was not in place. Tests are now added.

How was the bug found?

Reported by a team member on GitHub.

OmarTawfik · 2018-04-05T01:53:41Z

Add tests:

errors when parsing resource descriptors in both languages
path too long in PathUtilities. #Resolved

AlekseyTs · 2018-04-05T02:25:30Z

@OmarTawfik What bug is this change fixing? #Resolved

AlekseyTs · 2018-04-05T02:31:41Z

src/Compilers/Core/Portable/Emit/EmitOptions.cs

+
+            if (PdbFilePath != null && !PathUtilities.IsValidFilePath(PdbFilePath))
+            {
+                diagnostics.Add(messageProvider.CreateDiagnostic(messageProvider.FTL_InputFileNameTooLong, Location.None, PdbFilePath));


FTL_InputFileNameTooLong [](start = 81, length = 24)

We probably should use name that is more accurately reflects the message. For example, FTL_InvalidInputFileName. #Closed

Let's open a separate issue to follow up to avoid possible extra work for LOC team in dev15.7.x.

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

Created #25944

In reply to: 179337455 [](ancestors = 179337455,179337190)

OmarTawfik · 2018-04-05T02:36:55Z

@AlekseyTs this is fixing #23525
I'll add a few tests and then tag you/compiler-team for review/mark it as ready. #Closed

AlekseyTs · 2018-04-05T02:44:05Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

+        public void InvalidPathCharacterInPathMap()
+        {
+            string filePath = Temp.CreateFile().WriteAllText("").Path;
+            var compiler = new MockCSharpCompiler(null, _baseDirectory, new[]


MockCSharpCompiler [](start = 31, length = 18)

Pass "/preferreduilang:en" switch otherwise the test is likely to fail on non-english machine #Closed

AlekseyTs · 2018-04-05T02:44:17Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

+        public void InvalidPathCharacterInPdbPath()
+        {
+            string filePath = Temp.CreateFile().WriteAllText("").Path;
+            var compiler = new MockCSharpCompiler(null, _baseDirectory, new[]


Same comment #Closed

AlekseyTs · 2018-04-05T02:45:04Z

src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb

+        <WorkItem(23525, "https://github.com/dotnet/roslyn/issues/23525")>
+        Public Sub InvalidPathCharacterInPathMap()
+            Dim filePath = Temp.CreateFile().WriteAllText("").Path
+            Dim compiler = New MockVisualBasicCompiler(Nothing, _baseDirectory,


MockVisualBasicCompiler [](start = 31, length = 23)

Pass "/preferreduilang:en" #Closed

AlekseyTs · 2018-04-05T02:46:20Z

src/Compilers/VisualBasic/Test/Emit/PDB/PDBTests.vb

+                Dim result = Compilation.Emit(outStream, options:=New EmitOptions(pdbFilePath:="test\\?.pdb", debugInformationFormat:=DebugInformationFormat.Embedded))
+
+                Assert.False(result.Success)
+                result.Diagnostics.Verify(Diagnostic(ERRID.FTL_InputFileNameTooLong).WithArguments("test\\?.pdb").WithLocation(1, 1))


Diagnostic(ERRID.FTL_InputFileNameTooLong).WithArguments("test\?.pdb").WithLocation(1, 1) [](start = 42, length = 90)

Please include the actual error message in a comment as we usually do. #Closed

AlekseyTs · 2018-04-05T02:52:22Z

It looks like FTL_InputFileNameTooLong error is reported in more places than were adjusted. Are the other places doing proper validation? #Closed

OmarTawfik · 2018-04-05T02:57:40Z

The places where invalid paths are checked and reported.

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

OmarTawfik · 2018-04-05T03:04:50Z

@dotnet/roslyn-compiler this is ready for review.
@AlekseyTs thanks for taking an early look :)

jaredpar · 2018-04-05T16:39:01Z

approved pending @AlekseyTs sign off.

jcouv

LGTM

AlekseyTs · 2018-04-05T17:50:41Z

src/Compilers/Core/Portable/FileSystem/PathUtilities.cs

+        {
+            try
+            {
+                if (string.IsNullOrEmpty(path))


if (string.IsNullOrEmpty(path)) [](start = 16, length = 31)

Not very important, but this if can be extracted out of try.

AlekseyTs · 2018-04-05T17:52:34Z

src/Compilers/Core/Portable/FileSystem/PathUtilities.cs

+                var fileInfo = new FileInfo(path);
+                return !string.IsNullOrEmpty(fileInfo.Name);
+            }
+            catch (Exception ex) when (


when ( [](start = 33, length = 6)

Any particular reason to use when clause instead of using catch clause per specific exception type?

Just a readability preference.

AlekseyTs

LGTM (iteration 3)

OmarTawfik added Bug Area-Compilers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Apr 3, 2018

OmarTawfik added this to the 15.7 milestone Apr 3, 2018

OmarTawfik self-assigned this Apr 3, 2018

OmarTawfik requested a review from a team as a code owner April 3, 2018 00:49

OmarTawfik removed the request for review from a team April 3, 2018 00:56

OmarTawfik changed the title ~~Produce diagnostics on invalid pdb file path instead of crashing~~ Use PathUtilities for calculating pdb name instead of System.IO.Path Apr 3, 2018

OmarTawfik changed the title ~~Use PathUtilities for calculating pdb name instead of System.IO.Path~~ Produce errors on invalid pdbpath supplied to compiler Apr 4, 2018

Produce errors on invalid pdbpath supplied to compiler

0730892

AlekseyTs reviewed Apr 5, 2018

View reviewed changes

OmarTawfik mentioned this pull request Apr 5, 2018

Nit: Resource change #25944

Closed

More tests

e032144

OmarTawfik removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 5, 2018

OmarTawfik requested a review from a team April 5, 2018 02:58

Merge branch 'dev15.7.x' into fix-23525-invalid-path-character

43c5a8a

jaredpar approved these changes Apr 5, 2018

View reviewed changes

jaredpar added the Approved to merge label Apr 5, 2018

jcouv approved these changes Apr 5, 2018

View reviewed changes

AlekseyTs reviewed Apr 5, 2018

View reviewed changes

AlekseyTs approved these changes Apr 5, 2018

View reviewed changes

OmarTawfik merged commit d23cc53 into dotnet:dev15.7.x Apr 5, 2018

OmarTawfik deleted the fix-23525-invalid-path-character branch April 5, 2018 18:41

OmarTawfik added a commit that referenced this pull request Apr 13, 2018

Change resource string (feedback from #25903) (#26131)

d187f90

Conversation

OmarTawfik commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Uh oh!

OmarTawfik commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Apr 5, 2018 • edited by OmarTawfik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

OmarTawfik Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

OmarTawfik commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OmarTawfik commented Apr 5, 2018

Uh oh!

OmarTawfik commented Apr 5, 2018

Uh oh!

jaredpar commented Apr 5, 2018

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

OmarTawfik Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

OmarTawfik commented Apr 3, 2018 •

edited

Loading

OmarTawfik commented Apr 5, 2018 •

edited

Loading

AlekseyTs commented Apr 5, 2018 •

edited by OmarTawfik

Loading

AlekseyTs Apr 5, 2018 •

edited

Loading

OmarTawfik commented Apr 5, 2018 •

edited

Loading

AlekseyTs Apr 5, 2018 •

edited

Loading

AlekseyTs Apr 5, 2018 •

edited

Loading

AlekseyTs Apr 5, 2018 •

edited

Loading

AlekseyTs Apr 5, 2018 •

edited

Loading

AlekseyTs commented Apr 5, 2018 •

edited

Loading