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.

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

Deserializing TimeZoneInfo on Linux throws The Month parameter must be in the range 1 through 12 #30679

Closed
o-mdr opened this issue Aug 26, 2019 · 7 comments · Fixed by dotnet/coreclr#26405
Assignees
Milestone

Comments

@o-mdr
Copy link

o-mdr commented Aug 26, 2019

Issue Title

Deserializing TimeZoneInfo on Linux using BinaryFormatter throws The Month parameter must be in the range 1 through 12

General

.net core 2.2, Ubuntu 18 LTS. Please note the same code works fine on Windows. This appears to be a linux specific issue.

I am migrating a library from .net framework 4.7 to .net core 2.2 and found an issue with deep object cloning that I narrowed down to a short reproducible code snippet below.

Try this yourself:

using System;
using System.IO;
using System.Runtime.Serialization.Formatters.Binary;

namespace Test
{
    public class Program
    {
        public static void Main(string[] args)
        {
            // any zone here, don't care what it is
            var zone = TimeZoneInfo.GetSystemTimeZones()[0];			
            var formatter = new BinaryFormatter();
           
            using (MemoryStream stream = new MemoryStream())
            {
                formatter.Serialize(stream, zone);
                stream.Seek(0, SeekOrigin.Begin);
                var result = formatter.Deserialize(stream); // On Linux, exception is on this line <<<<<
                Console.WriteLine("all ok");
            }
        }
    }
}

On Windows platforms using .net core 2.2 this works fine, but I get an exception on Linux platforms:

Unhandled Exception: System.Runtime.Serialization.SerializationException: An error occurred while deserializing the object.  The serialized data is corrupt. ---> System.ArgumentOutOfRangeException: The Month parameter must be in the range 1 through 12.
Parameter name: month
   at System.TimeZoneInfo.TransitionTime.ValidateTransitionTime(DateTime timeOfDay, Int32 month, Int32 week, Int32 day, DayOfWeek dayOfWeek)
   at System.TimeZoneInfo.TransitionTime.System.Runtime.Serialization.IDeserializationCallback.OnDeserialization(Object sender)
   --- End of inner exception stack trace ---
   at System.TimeZoneInfo.TransitionTime.System.Runtime.Serialization.IDeserializationCallback.OnDeserialization(Object sender)
   at System.Runtime.Serialization.ObjectManager.RaiseDeserializationEvent()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream)
   at Test.Program.Main(String[] args)

Asked on StackOverflow https://stackoverflow.com/q/57655469/706456

@scalablecory scalablecory transferred this issue from dotnet/core Aug 26, 2019
@scalablecory
Copy link
Contributor

@tarekgh

@stephentoub
Copy link
Member

cc: @ViktorHofer, @eerhardt

@eerhardt
Copy link
Member

@ViktorHofer - I'll take a look at this. My assumption from looking at the code is that the fields are not getting set correctly during deserialization. I assume _month is 0 for some reason:

https://github.com/dotnet/corefx/blob/1841042b99062de13dc80098cede9413be569238/src/Common/src/CoreLib/System/TimeZoneInfo.TransitionTime.cs#L109-L122

@eerhardt eerhardt self-assigned this Aug 26, 2019
@eerhardt
Copy link
Member

So I did some debugging (and some reading code to jog my memory), and here's what I found.

When we create TimeZoneInfo's on Unix, we almost always ignore the TransitionTime objects. This is because the way time zones work on Unix is very different than how they work on Windows.

On Windows, we have a couple small rules for each time zone that say how and when the Daylight Savings Time transitions (if any for the zone). The TransitionTime encodes rules like "DST starts on the 3rd Sunday of March", and then we have logic in TimeZoneInfo that interprets these rules.

On Unix, it works differently, and every time there is a DST transition, there is a new AdjustmentRule instance created. On Unix, we set a flag on AdjustmentRule called NoDaylightTransitions, which tells the rest of the logic to ignore any transitions between the Start and End of the AdjustmentRule. Thus, when NoDaylightTransitions is true, the TransitionTime objects are set to default:

https://github.com/dotnet/corefx/blob/fb30a2d1a2bffbba2c039fd294c471962576a2d7/src/Common/src/CoreLib/System/TimeZoneInfo.Unix.cs#L949-L972

So when we serialize a TimeZoneInfo on Unix, these default TransitionTime objects are getting serialized into the stream. Then when they are deserialized, the ValidateTransitionTime method above gets invoked on default instances and it throws because _month, _week, etc are set to 0 which is normally not valid.

The easiest fix I can think of is to check during OnDeserialization if the TransitionTime equals the default, and if it is, skip calling ValidateTransitionTime. We shouldn't change ValidateTransitionTime itself, since it is called from other places that still want to guard against 0 values.

@tarekgh
Copy link
Member

tarekgh commented Aug 26, 2019

The easiest fix I can think of is to check during OnDeserialization if the TransitionTime equals the default, and if it is, skip calling ValidateTransitionTime.

I think this is reasonable as I don't think anyone can create and force default value in their custom rules before serialization.

@eerhardt
Copy link
Member

Thanks @tarekgh, I’ll make that PR (and add tests) tomorrow.

eerhardt referenced this issue in eerhardt/corefx Aug 27, 2019
@jkotas jkotas closed this as completed Aug 28, 2019
jonpryor referenced this issue in dotnet/android Dec 3, 2019
Changes: mono/api-snapshot@fc50bc4...45a61d9

        $ git diff --shortstat fc50bc4f...45a61d93
         22 files changed, 775 insertions(+), 474 deletions(-)

Changes: dotnet/cecil@a6c8f5e...a6a7f5c

        $ git diff --shortstat a6c8f5e1...a6a7f5c0
         55 files changed, 818 insertions(+), 530 deletions(-)

Changes: mono/corefx@1f87de3...49f1c45

        $ git diff --shortstat e4f7102b...49f1c453
         38 files changed, 1171 insertions(+), 419 deletions(-)

Changes: dotnet/linker@ebe2a1f...e8d054b

        $ git diff --shortstat ebe2a1f4...e8d054bf
         137 files changed, 5360 insertions(+), 1781 deletions(-)

Changes: mono/mono@8946e49...18920a8

        $ git diff --shortstat 8946e49a...18920a83
         1811 files changed, 47240 insertions(+), 48331 deletions(-)

Changes: xamarin/xamarin-android-api-compatibility@a61271e...50a3c52

        $ git diff --shortstat a61271e0...50a3c52d
         1 file changed, 2 insertions(+), 791 deletions(-)

Fixes: #3619

Context: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1005448
Context: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/967582
Context: https://github.com/dotnet/coreclr/issues/26370
Context: https://github.com/dotnet/coreclr/issues/26479
Context: https://github.com/dotnet/corefx/issues/40455
Context: https://github.com/dotnet/corefx/issues/40578
Context: mono/mono#7377
Context: mono/mono#12421
Context: mono/mono#12586
Context: mono/mono#14080
Context: mono/mono#14725
Context: mono/mono#14772
Context: mono/mono#15261
Context: mono/mono#15262
Context: mono/mono#15263
Context: mono/mono#15307
Context: mono/mono#15308
Context: mono/mono#15310
Context: mono/mono#15646
Context: mono/mono#15687
Context: mono/mono#15805
Context: mono/mono#15992
Context: mono/mono#15994
Context: mono/mono#15999
Context: mono/mono#16032
Context: mono/mono#16034
Context: mono/mono#16046
Context: mono/mono#16192
Context: mono/mono#16308
Context: mono/mono#16310
Context: mono/mono#16369
Context: mono/mono#16380
Context: mono/mono#16381
Context: mono/mono#16395
Context: mono/mono#16411
Context: mono/mono#16415
Context: mono/mono#16486
Context: mono/mono#16570
Context: mono/mono#16605
Context: mono/mono#16616
Context: mono/mono#16689
Context: mono/mono#16701
Context: mono/mono#16712
Context: mono/mono#16742
Context: mono/mono#16759
Context: mono/mono#16803
Context: mono/mono#16808
Context: mono/mono#16824
Context: mono/mono#16876
Context: mono/mono#16879
Context: mono/mono#16918
Context: mono/mono#16943
Context: mono/mono#16950
Context: mono/mono#16974
Context: mono/mono#17004
Context: mono/mono#17017
Context: mono/mono#17038
Context: mono/mono#17040
Context: mono/mono#17083
Context: mono/mono#17084
Context: mono/mono#17133
Context: mono/mono#17139
Context: mono/mono#17151
Context: mono/mono#17180
Context: mono/mono#17278
Context: mono/mono#17549
Context: mono/mono#17569
Context: mono/mono#17665
Context: mono/mono#17687
Context: mono/mono#17737
Context: mono/mono#17790
Context: mono/mono#17924
Context: mono/mono#17931
Context: https://github.com/mono/mono/issues/26758
Context: https://github.com/mono/mono/issues/37913
Context: xamarin/xamarin-macios#7005
jonpryor referenced this issue in dotnet/android Dec 3, 2019
Changes: mono/api-snapshot@fc50bc4...45a61d9

        $ git diff --shortstat fc50bc4f...45a61d93
         22 files changed, 775 insertions(+), 474 deletions(-)

Changes: dotnet/cecil@a6c8f5e...a6a7f5c

        $ git diff --shortstat a6c8f5e1...a6a7f5c0
         55 files changed, 818 insertions(+), 530 deletions(-)

Changes: mono/corefx@1f87de3...49f1c45

        $ git diff --shortstat e4f7102b...49f1c453
         38 files changed, 1171 insertions(+), 419 deletions(-)

Changes: dotnet/linker@ebe2a1f...e8d054b

        $ git diff --shortstat ebe2a1f4...e8d054bf
         137 files changed, 5360 insertions(+), 1781 deletions(-)

Changes: mono/mono@8946e49...18920a8

        $ git diff --shortstat 8946e49a...18920a83
         1811 files changed, 47240 insertions(+), 48331 deletions(-)

Changes: xamarin/xamarin-android-api-compatibility@a61271e...50a3c52

        $ git diff --shortstat a61271e0...50a3c52d
         1 file changed, 2 insertions(+), 791 deletions(-)

Fixes: #3619

Context: https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1005448
Context: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems/edit/967582
Context: https://github.com/dotnet/coreclr/issues/26370
Context: https://github.com/dotnet/coreclr/issues/26479
Context: https://github.com/dotnet/corefx/issues/40455
Context: https://github.com/dotnet/corefx/issues/40578
Context: mono/mono#7377
Context: mono/mono#12421
Context: mono/mono#12586
Context: mono/mono#14080
Context: mono/mono#14725
Context: mono/mono#14772
Context: mono/mono#15261
Context: mono/mono#15262
Context: mono/mono#15263
Context: mono/mono#15307
Context: mono/mono#15308
Context: mono/mono#15310
Context: mono/mono#15646
Context: mono/mono#15687
Context: mono/mono#15805
Context: mono/mono#15992
Context: mono/mono#15994
Context: mono/mono#15999
Context: mono/mono#16032
Context: mono/mono#16034
Context: mono/mono#16046
Context: mono/mono#16192
Context: mono/mono#16308
Context: mono/mono#16310
Context: mono/mono#16369
Context: mono/mono#16380
Context: mono/mono#16381
Context: mono/mono#16395
Context: mono/mono#16411
Context: mono/mono#16415
Context: mono/mono#16486
Context: mono/mono#16570
Context: mono/mono#16605
Context: mono/mono#16616
Context: mono/mono#16689
Context: mono/mono#16701
Context: mono/mono#16712
Context: mono/mono#16742
Context: mono/mono#16759
Context: mono/mono#16803
Context: mono/mono#16808
Context: mono/mono#16824
Context: mono/mono#16876
Context: mono/mono#16879
Context: mono/mono#16918
Context: mono/mono#16943
Context: mono/mono#16950
Context: mono/mono#16974
Context: mono/mono#17004
Context: mono/mono#17017
Context: mono/mono#17038
Context: mono/mono#17040
Context: mono/mono#17083
Context: mono/mono#17084
Context: mono/mono#17133
Context: mono/mono#17139
Context: mono/mono#17151
Context: mono/mono#17180
Context: mono/mono#17278
Context: mono/mono#17549
Context: mono/mono#17569
Context: mono/mono#17665
Context: mono/mono#17687
Context: mono/mono#17737
Context: mono/mono#17790
Context: mono/mono#17924
Context: mono/mono#17931
Context: https://github.com/mono/mono/issues/26758
Context: https://github.com/mono/mono/issues/37913
Context: xamarin/xamarin-macios#7005
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@ivmazurenko
Copy link

Hello, I have a similar issue in xamarin android project,
dotnet/android#5205

Any chance to avoid this crash? Maybe you have any recommendations?

@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants