Rewrite SafeRoleEnvironment to not use Assembly.LoadWithPartialName #339
Conversation
scottbommarito
requested review from
joelverhagen,
agr,
loic-sharma,
xavierdecoster,
shishirx34,
ryuyu,
chenriksson,
cristinamanum and
skofman1
|
Deploying to DEV to test. |
|
Works great on DEV--functional tests passed and no exceptions or failed requests in AI. |
| } | ||
| catch (Exception e) | ||
| { | ||
| // If the assembly is not available, a file load exception will be thrown. | ||
| // Otherwise, it must be an exception thrown by the loaded assembly itself. | ||
| if (!(e is FileNotFoundException || e is FileLoadException || e is BadImageFormatException)) | ||
| { | ||
| throw; | ||
| } |
There was a problem hiding this comment.
This try-catch will be expensive if these methods are called often. We should try to call once, and if we get one of these exceptions, we should enable a circuit breaker.
There was a problem hiding this comment.
added...deploying to DEV again to test
| { | ||
| return RoleEnvironment.GetLocalResource(name).RootPath; | ||
| return TryGetField(() => RoleEnvironment.GetLocalResource(name).RootPath, out path); |
There was a problem hiding this comment.
Can GetLocalResource return null? What is the expected behaviour in that case?
There was a problem hiding this comment.
If GetLocalResource is null, it will throw a NullReferenceException, which will be bubbled up to the caller of this method. This is the same behavior as before.
| /// <returns>Loaded assembly, if any.</returns> | ||
| private static Assembly GetServiceRuntimeAssembly() | ||
| private static bool _assemblyIsAvailable = true; | ||
| private static bool TryGetField<T>(Func<T> getValue, out T value) |
There was a problem hiding this comment.
no need for generics here. The return value is always string.
There was a problem hiding this comment.
Good catch.
The idea, however, is that this will be a proxy for every use of RoleEnvironment, so keeping it like this will make it easier for us to add additional methods if we need to access fields of RoleEnvironment that do not return a string.
If I had noticed that all of our uses of RoleEnvironment were strings initially I would have made it a Func<string>, but at this point there's no reason for the change.
There was a problem hiding this comment.
This introduces an unnecessary complexity to the code. Agree that it's not a bug deal.
|
How did you test the code (besides deploying to dev)? |
|
I deployed it to DEV (where I also ran it locally (where |
* Feed2Catalog: replace inner loops with one outer loop (#329) Resolve NuGet/Engineering#1526. * V3: rename asynchronous IStorage methods (#336) Progress on NuGet/NuGetGallery#6267. * Test: fix flaky basic search tests (#337) Progress on NuGet/NuGetGallery#6292. * Add signing to search service (#338) Progress on https://github.com/NuGet/Engineering/issues/1644 * Test: add registration tests (#341) Progress on NuGet/NuGetGallery#6317. * remove unused scripts (#340) * MonitoringProcessor: improve throughput (#342) Progress on NuGet/NuGetGallery#6327. * Catalog2Dnx: improve throughput (#335) Progress on NuGet/NuGetGallery#6267. * Functional tests should not be dependent on a specific hash and file size of the test package (#343) * Add MicroBuild dependency and signing of output DLLs (#345) Progress on https://github.com/NuGet/Engineering/issues/1644 * Rewrite SafeRoleEnvironment to not use Assembly.LoadWithPartialName (#339) * Test: improve and add registration tests (#346) More progress on NuGet/NuGetGallery#6317. * Use ServerCommon commit that is consistent with other repositories (#347) Progress on https://github.com/NuGet/Engineering/issues/1644 * [Monitoring] Ensure packages are signed (#348) This adds a validation to ensure indexed packages are signed by verifying that the package contains a [package signature file](https://github.com/NuGet/Home/wiki/Package-Signatures-Technical-Details#-the-package-signature-file). This validation is disabled for now. To enable this validation, the flag `-expectsSignature true` must be added to ng.exe's command line arguments. This validation will be enabled once all packages are repository signed. Part of NuGet/Engineering#1627 * V3: make stylistic consistency and cleanup changes (#349) Progress on NuGet/NuGetGallery#6411.
Assembly.LoadWithPartialNameis insecure (as caught by Roslyn).Fortunately, we don't have to use reflection at all, because we can simply
try-catchtheRoleEnvironmentaccesses instead.