Fixed crashes on some SOAP 1.2 requests when using GetBufferedInputStream

[theiji]

This fixes issue 1759 crashes on some SOAP 1.2 requests when using GetBufferedInputStream

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-09-13T17:44:04.444+0000

• Duration: 65 min 52 sec

Test stats 🧪

Test	Results
Failed	0
Passed	16518
Skipped	164
Total	16682

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run benchmark tests : Run the benchmark test.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[z1c0]

Thanks for your contribution @theiji!
May I ask you to add a reproducer/test to your PR, please?
You can find existing tests for SOAP requests here.

Update:

I'm currently working on Request Body capturing for .NET Framework (#379) and the underlying issue here is an interesting test case in this scenario (see also #1759 (comment)).
A reproducer or test case would be highly appreciated so we can include this PR as part of the mentioned feature (#379).

[z1c0]

@theiji I was able to verify your fix - thanks again!
I think I could simply the code a little bit and added an additional test (small & large payload).
May I ask to to update your PR the way I suggested in my branch https://github.com/elastic/apm-agent-dotnet/tree/1759-bug-elasticapmaspnetfullframework-crashes-on-some-soap-requests-to-asp-net-web-service?

[z1c0]

FYI @gregkalapos - I was able to reproduce the issue and verify the fix provided here.
The underlying problem is that in GetSoap12ActionFromInputStream we use an XmlReader to read the posted payload data. For large payloads, we end up in a situation where the XmlReader only reads part of the stream (as large as its internal buffer size).

This leaves the underlying stream in a "half-read" state that, later on, causes the internal server error. By making sure that the stream is read to the end at all times, this issue is fixed.

[cla-checker-service]

💚 CLA has been signed

[z1c0]

LGTM!
(It looks like you still need to sign the CLA though @theiji)

[gregkalapos]

LGTM as well.

@theiji please make sure you use the same e-mail address that you used for your commits when you sign the CLA - it’s here in the 2. line.

We can merge once the CLA is signed.

[theiji]

@gregkalapos @z1c0 sorry. CLA signed

[gregkalapos]

Thanks for signing the CLA @theiji.

There is one minor issue with the changed test - details below. Sorry for missing that previously; I just noticed that after CI ran the PR (it wasn't running the PR previously due to the missing CLA).

[gregkalapos]

Suggested change

	[AspNetFullFrameworkFact]
	[AspNetFullFrameworkTheory]

Currently it fails:

System.InvalidOperationException : [Fact] methods are not allowed to have parameters. Did you mean to use [Theory]?

Due to the new parameter, it needs to be AspNetFullFrameworkTheory

[theiji]

@gregkalapos done

[z1c0]

Thanks for hanging on @theiji - merge is imminent :-)

[gregkalapos]

Thanks for your contribution @theiji!