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

ILSpy should not crash if fullName contains no "/" - no version info #2227

Merged
merged 1 commit into from
Nov 24, 2020
Merged

ILSpy should not crash if fullName contains no "/" - no version info #2227

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion ICSharpCode.Decompiler/Metadata/DotNetCorePathFinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,14 @@ public DotNetCorePackageInfo(string fullName, string type, string path, string[]
{
var parts = fullName.Split('/');
this.Name = parts[0];
this.Version = parts[1];
if (parts.Length > 1)
{
this.Version = parts[1];
} else
{
this.Version = "<UNKNOWN>";
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure whether this is a good idea... The Version property is used at:

ILSpy/ICSharpCode.Decompiler/Metadata/DotNetCorePathFinder.cs

Lines 109 to 115 in dd98de8

foreach (var item in p.RuntimeComponents)
{
var itemPath = Path.GetDirectoryName(item);
var fullPath = Path.Combine(path, p.Name, p.Version, itemPath).ToLowerInvariant();
if (Directory.Exists(fullPath))
packageBasePaths.Add(fullPath);
}

Using <UNKNOWN> instead of a valid version will not work here... > and < are not valid in path names on Windows. Do you have an example were fullName does not contain a /? If yes, how would the correct package path be derived from incomplete information? Wouldn't it make more sense to exclude these incomplete package descriptions from the list of packages in our case? That is, never call the DotNetCorePackageInfo ctor in the first place?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I just debugged my application written with Oxygene (Object Pascal for DotNet).
The IL View works fine, but after switching CSharp I got thoses Error messages.

This fix works because Directory.Exists(fullPath) simply returns false (which is true - there is no such directory).

If you look in the Delphi.deps.json you can find the following content:

{
  "runtimeTarget": {
    "name": ".NETStandard,Version=v2.0",
    "signature": "ddefb372be77579a331f6c0a0acd01dbb98e3ce1"
  },
  "compilationOptions": {
  },
  "targets": {
    ".NETStandard,Version=v2.0": {
      "Delphi/1.0.0": {
        "dependencies": {
        },
        "runtime": {
          "Delphi.dll": {
          },
          "Echoes.dll": {
          },
          "Elements.dll": {
          }
        }
      }
    }
  },
  "libraries": {
    "Delphi/1.0.0": {
      "type": "project",
      "serviceable": false,
      "sha512": ""
    },
    "Echoes": {
      "type": "project",
      "serviceable": false,
      "sha512": ""
    },
    "Elements": {
      "type": "project",
      "serviceable": false,
      "sha512": ""
    }
  }
}

As you can see, in the libraries node you have some entries without any version info. Those libraries are not provided via nuget - they are just stored next to the "Delphi.dll".
As you say, this is maybe an invalid configuration - but I think ILSpy should not crash in those cases.

Copy link
Member

Choose a reason for hiding this comment

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

This fix works because Directory.Exists(fullPath) simply returns false (which is true - there is no such directory).

Yes, but we already know that it cannot possibly exist... why waste CPU cycles and disk IO trying the impossible?

As you can see, in the libraries node you have some entries without any version info. Those libraries are not provided via nuget - they are just stored next to the "Delphi.dll".
As you say, this is maybe an invalid configuration - but I think ILSpy should not crash in those cases.

I don't know enough about Oxygene, but I think, we should be fine without these entries... The decompiler will automatically resolve any assemblies next to the main assembly anyway. If the resolver cannot find them, you can always add these assemblies manually and ILSpy will pick them up.

Copy link
Member

Choose a reason for hiding this comment

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

fyi... the build is failing because the code you changed is no longer formatted properly... are you OK with me fixing this before the merge?

Copy link
Author

Choose a reason for hiding this comment

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

yes, of course

}

this.Type = type;
this.Path = path;
this.RuntimeComponents = runtimeComponents ?? Empty<string>.Array;
Expand Down