Avoid non documented exceptions in BinaryFormatter.Deserialize #40215
Conversation
…ceptions than those specified in the documentation (only supposed to throw SerializationException or SecurityException).
|
|
…f BinaryFormatter.Deserialize. Fix naming error in SerializationGuardTests.
|
Any feedback on this? This is my first pull request - so not sure if I did everything right? :) |
|
cc: @ViktorHofer |
|
@ViktorHofer I think thats a valid concern, but I guess that would also be relying on undocumented/undefined behaviour? If nothing else with the change the consumer of the API still has access to the internal exception via innerException. |
Right, but we should still make sure that we know the impact of a breaking change. I don't know how many customers currently rely on this undocumented behavior. We might want to gather some data before merging. cc @leecow |
|
I think this warrants a breaking change announcement on https://github.com/dotnet/announcements/issues. |
|
Hi @leecow, |
|
Yes. @ViktorHofer, can you help with that when the time comes? |
|
@ViktorHofer what's next steps here? One option is: take the change, since we have some time until we ship, and we will have many preview releases on the way. |
|
Yes, taking the change now in an early stage sounds reasonable. And yes, I can help with the announcement. |
|
/azp run corefx-ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run corefx-ci |
|
Commenter does not have sufficient privileges for PR 40215 in repo dotnet/corefx |
|
The failing test is https://github.com/dotnet/corefx/issues/29742#issuecomment-539585555. I'm fine with merging this. |
|
Thanks a lot @birkmose! |
|
@ViktorHofer I was actually trying to reproduce right now in my personal tree, but didn't seem to be related to this PR - not BinarySerializer references here :) |
…0191022.10 - Microsoft.NETCore.App - 5.0.0-alpha1.19522.10 Dependency coherency updates - Microsoft.NETCore.Platforms - 5.0.0-alpha1.19521.4 (parent: Microsoft.NETCore.App) - Microsoft.Win32.Registry - 5.0.0-alpha1.19521.4 (parent: Microsoft.NETCore.App) - Microsoft.Win32.SystemEvents - 5.0.0-alpha1.19521.4 (parent: Microsoft.NETCore.App) - System.CodeDom - 5.0.0-alpha1.19521.4 (parent: Microsoft.NETCore.App) - System.Configuration.ConfigurationManager - 5.0.0-alpha1.19521.4 (parent: Microsoft.NETCore.App) - System.Drawing.Common - 5.0.0-alpha1.19521.4 (parent: Microsoft.NETCore.App) - System.Resources.Extensions - 5.0.0-alpha1.19521.4 (parent: Microsoft.NETCore.App) - System.Security.Cryptography.Cng - 5.0.0-alpha1.19521.4 (parent: Microsoft.NETCore.App) - System.Security.Permissions - 5.0.0-alpha1.19521.4 (parent: Microsoft.NETCore.App) - System.Windows.Extensions - 5.0.0-alpha1.19521.4 (parent: Microsoft.NETCore.App) - Microsoft.NET.Sdk.IL - 5.0.0-alpha1.19522.3 (parent: Microsoft.NETCore.App) - Microsoft.NETCore.ILAsm - 5.0.0-alpha1.19522.3 (parent: Microsoft.NETCore.App) Fix tests to account for dotnet/corefx#40215
|
Hi all. |
…t/corefx#40215) * dotnet/corefx#35491 Fixes issue with BinaryFormatter.Deserialize throwing other exceptions than those specified in the documentation (only supposed to throw SerializationException or SecurityException). * Update UnitySerializationHolderTests to test the expected behaviour of BinaryFormatter.Deserialize. Fix naming error in SerializationGuardTests. Commit migrated from dotnet/corefx@1736bfa
Fixes #35491. BinaryFormatter.Deserialize can throw other exceptions than those specified in the documentation (SerializationException, SecurityException & ArgumentNullException) in cases of corrupt input. This update changes the BinaryFormatter.Deserialize method to wrap those "internal" exception inside an SerializationException.