Fix OverflowException in JsonSerializer.Serialize

[felipepessoto]

Fix #609

[azure-pipelines]

Commenter does not have sufficient privileges for PR 1308 in repo dotnet/runtime

[danmoseley]

/azp run

[azure-pipelines]

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

[stephentoub]

@ahsonkhan, @steveharter, can you take a look as well? Thanks.

[ahsonkhan]

Fix #609

We should consider making a similar fix to the public ArrayBufferWriter as well, based on where we land in this PR.

runtime/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs

Lines 168 to 180 in fd7a5e8

	if (sizeHint > FreeCapacity)
	{
	int growBy = Math.Max(sizeHint, _buffer.Length);
	if (_buffer.Length == 0)
	{
	growBy = Math.Max(growBy, DefaultInitialBufferSize);
	}
	int newSize = checked(_buffer.Length + growBy);
	Array.Resize(ref _buffer, newSize);
	}

[stephentoub]

@felipepessoto, sorry for the delay. This looks good and ready to go, but there's a conflict in the tests now... can you rebase to address it?

[felipepessoto]

@felipepessoto, sorry for the delay. This looks good and ready to go, but there's a conflict in the tests now... can you rebase to address it?

Yes sure. But I still have some pending things to do. I still didn't stop to check how can I restrict the test to 64bits, and I couldn't run the coverage test yet, last time it took hours and didn't finish

[stephentoub]

I still didn't stop to check how can I restrict the test to 64bits

e.g.

runtime/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs

Line 723 in 0f834db

[ConditionalFact(nameof(IsX64))]

runtime/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs

Line 27 in 0f834db

public static bool IsX64 { get; } = IntPtr.Size >= 8;

[ahsonkhan]

I couldn't run the coverage test yet, last time it took hours and didn't finish

You are probably building using a debug coreclr which causes tests to take a long time to run. If you build coreclr for release and then build the libraries using the release coreclr, the tests should run faster.

I was able to run the coverage after merging master into your branch (by changing your test to be part of inner loop, removed outerloop from attribute):
From root of repo run the following:
build.cmd -subsetCategory coreclr -c Release && build.cmd -subsetCategory libraries /p:CoreCLRConfiguration=Release
Then navigate to src\libraries\System.Text.Json\tests and run:
dotnet msbuild /t:BuildAndTest /p:Coverage=true

See #839 (comment)

cc @safern, @ViktorHofer - do we need to increase the visibility of this issue so fewer folks hit against running tests with debug coreclr, by default?

Looks like all the new branches introduced in this PR are covered by the test :)

The path that is likely not covered (and this sequence of code flow won't show up in test coverage):
(uint)(currentLength + growBy) > int.MaxValue is true but (uint)(currentLength + sizeHint) > int.MaxValue is false

If you do run all outerloop tests, it will take a lot longer (~10 minutes for me, but shouldn't take hours):
dotnet msbuild /t:RebuildAndTest /p:Coverage=true /p:Outerloop=true

===========================================================================================================
    Discovering: System.Text.Json.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.Text.Json.Tests (found 1692 of 1703 test cases)
    Starting:    System.Text.Json.Tests (parallel test collections = on, max threads = 8)
    Finished:    System.Text.Json.Tests
  === TEST EXECUTION SUMMARY ===
     System.Text.Json.Tests  Total: 11482, Errors: 0, Failed: 0, Skipped: 0, Time: 365.595s

[felipepessoto]

I still didn't stop to check how can I restrict the test to 64bits

e.g.

runtime/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs

Line 723 in 0f834db

[ConditionalFact(nameof(IsX64))]

runtime/src/libraries/System.Text.Json/tests/Utf8JsonWriterTests.cs

Line 27 in 0f834db

public static bool IsX64 { get; } = IntPtr.Size >= 8;

Thx, done

[ahsonkhan]

@felipepessoto - are you up for making a similar fix to ArrayBufferWriter<T>, in a separate PR to fix/close the issue?

runtime/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs

Lines 158 to 183 in 4f71081

	private void CheckAndResizeBuffer(int sizeHint)
	{
	if (sizeHint < 0)
	throw new ArgumentException(nameof(sizeHint));
	if (sizeHint == 0)
	{
	sizeHint = 1;
	}
	if (sizeHint > FreeCapacity)
	{
	int growBy = Math.Max(sizeHint, _buffer.Length);
	if (_buffer.Length == 0)
	{
	growBy = Math.Max(growBy, DefaultInitialBufferSize);
	}
	int newSize = checked(_buffer.Length + growBy);
	Array.Resize(ref _buffer, newSize);
	}
	Debug.Assert(FreeCapacity > 0 && FreeCapacity >= sizeHint);
	}

[felipepessoto]

@felipepessoto - are you up for making a similar fix to ArrayBufferWriter<T>, in a separate PR to fix/close the issue?

runtime/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs

Lines 158 to 183 in 4f71081

	private void CheckAndResizeBuffer(int sizeHint)
	{
	if (sizeHint < 0)
	throw new ArgumentException(nameof(sizeHint));
	if (sizeHint == 0)
	{
	sizeHint = 1;
	}
	if (sizeHint > FreeCapacity)
	{
	int growBy = Math.Max(sizeHint, _buffer.Length);
	if (_buffer.Length == 0)
	{
	growBy = Math.Max(growBy, DefaultInitialBufferSize);
	}
	int newSize = checked(_buffer.Length + growBy);
	Array.Resize(ref _buffer, newSize);
	}
	Debug.Assert(FreeCapacity > 0 && FreeCapacity >= sizeHint);
	}

Absolutely