Let argument check run before aborting check.

[AaronRobinsonMSFT]

Move abort check after the invalid arg check.

Fixes #61783

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Move abort check after the invalid arg check.

Fixes #61783

Author:	AaronRobinsonMSFT
Assignees:	-
Labels:	area-System.Security
Milestone:	-

[AaronRobinsonMSFT]

/azp help

[azure-pipelines]

Supported commands

• help:
• Get descriptions, examples and documentation about supported commands
• Example: help "command_name"
• list:
• List all pipelines for this repository using a comment.
• Example: "list"
• run:
• Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
• Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
• where:
• Report back the Azure DevOps orgs that are related to this repository and org
• Example: "where"

See additional documentation.

[AaronRobinsonMSFT]

/azp list

[azure-pipelines]

CI/CD Pipelines for this repository:

• coreclr-gc-longrunning
• coreclr-gc-simulator
• runtime-coreclr outerloop
• runtime-coreclr jitstress
• runtime-coreclr jitstressregs
• runtime-coreclr jitstress2-jitstressregs
• runtime-coreclr gcstress0x3-gcstress0xc
• runtime-coreclr gcstress-extra
• runtime-coreclr r2r-extra
• runtime-coreclr jitstress-isas-x86
• runtime-coreclr jitstress-isas-arm
• runtime-coreclr jitstressregs-x86
• runtime-coreclr libraries-jitstressregs
• runtime-coreclr libraries-jitstress2-jitstressregs
• runtime-coreclr r2r
• runtime-coreclr runincontext
• runtime-coreclr crossgen2
• runtime-libraries-coreclr outerloop
• runtime-libraries-coreclr outerloop-windows
• runtime-libraries-coreclr outerloop-linux
• runtime-libraries-coreclr outerloop-osx
• runtime
• runtime-libraries enterprise-linux
• runtime-libraries stress-http
• runtime-libraries stress-ssl
• runtime-dev-innerloop
• runtime-coreclr crossgen2 outerloop
• coreclr-release-outerloop-nightly
• sync-runtime-to-mono
• runtime-coreclr crossgen2-composite
• runtime-jit-experimental
• runtime-coreclr libraries-jitstress
• runtime-coreclr libraries-gcstress0x3-gcstress0xc
• runtime-coreclr libraries-gcstress-extra
• dotnet-linker-tests
• runtime-coreclr ilasm
• runtime-coreclr crossgen2-composite gcstress
• runtime-libraries-mono outerloop
• runtime-staging
• runtime-richnav
• runtime-coreclr gc-largepages
• runtime-coreclr pgo
• runtime-coreclr libraries-pgo
• coreclr-gc-regions
• Antigen
• runtime-community
• Fuzzlyn
• runtime-coreclr superpmi-replay
• wasm-debugger
• runtime-coreclr superpmi-asmdiffs
• runtime-coreclr superpmi-asmdiff-checked-release
• runtime-manual

[vcsjones]

I'm a bit confused how this is the cause of the linked issue. The failure was in CryptoNative_HmacUpdate, but the changed function is for plain digesting, not HMAC.

What am I missing?

[AaronRobinsonMSFT]

@vcsjones Right. There are two issues actually in the same general area. The statement is for one and if one looks at the log it is for the other. Both are the same issue and I'm not sure why at this point, but the issue is around non-NULL for pointers even if the length is 0. The Android code path is much more aggressive about NULL as opposed to the other Linux approaches. I am looking into the other as well but wanted to try to run the Android leg to confirm with just this one.

[elinor-fung]

Might want to do a search for all instances of abort_if_invalid_pointer_argument - I suspect there may be others where the abort is happening on a null buffer before checking for a buffer length of 0.

[steveisok]

/azp run runtime-manual

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[AaronRobinsonMSFT]

/azp run runtime-manual

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[vcsjones]

I'm actually not entirely sure how this worked before. The hash one-shot pins source here:

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/HashProviderDispenser.OpenSsl.cs

Lines 60 to 65 in 899bf97

	fixed (byte* pSource = source)
	fixed (byte* pDestination = destination)
	{
	uint length = (uint)destination.Length;
	Check(Interop.Crypto.EvpDigestOneShot(evpType, pSource, source.Length, pDestination, &length));
	Debug.Assert(length == hashSize);

and feeds that in to EvpDigestOneShot. A fixed on an empty-span produces a null pointer. e.g. this prints true:

unsafe
{
    Span<byte> b = default;
    fixed (byte* pB = b)
    {
        Console.WriteLine(pB == null);
    }
}

So it goes to reason if you hash an empty span, source can be null:

runtime/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_evp.c

Lines 51 to 55 in 57bfe47

	int32_t CryptoNative_EvpDigestOneShot(intptr_t type, void* source, int32_t sourceSize, uint8_t* md, uint32_t* mdSize)
	{
	abort_if_invalid_pointer_argument (source);
	if (!type || !md || !mdSize || sourceSize < 0)

[AaronRobinsonMSFT]

I'm actually not entirely sure how this worked before.

@vcsjones That is exactly what I've been struggling with. I don't see how any of the generated changes caused this issue. I might just handle the NULL check for these methods. If you have a different suggestion on how best to fix the failure I am also open to that.

/cc @elinor-fung

[vcsjones]

abort_if_invalid_pointer_argument (source);

I don't know what changed recently to undercover this. Perhaps something kicked in when we consolidated everything to System.Security.Cryptography. There has been some major code shuffling lately (see #55690). So these tests in System.Security.Cryptography are only a few days or so old.

That assert doesn't make much sense now that I look at it. Here's a patch I came up with to correct these two asserts:

diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_evp.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_evp.c
index 3156d524503..04ff98209b0 100644
--- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_evp.c
+++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_evp.c
@@ -50,9 +50,7 @@ static jobject GetMessageDigestInstance(JNIEnv* env, intptr_t type)
 
 int32_t CryptoNative_EvpDigestOneShot(intptr_t type, void* source, int32_t sourceSize, uint8_t* md, uint32_t* mdSize)
 {
-    abort_if_invalid_pointer_argument (source);
-
-    if (!type || !md || !mdSize || sourceSize < 0)
+    if (!type || !md || !mdSize || sourceSize < 0 || (sourceSize > 0 && !source))
         return FAIL;
 
     JNIEnv* env = GetJNIEnv();
diff --git a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_hmac.c b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_hmac.c
index 7632eed5905..70f3030713a 100644
--- a/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_hmac.c
+++ b/src/libraries/Native/Unix/System.Security.Cryptography.Native.Android/pal_hmac.c
@@ -82,10 +82,12 @@ int32_t CryptoNative_HmacReset(jobject ctx)
 
 int32_t CryptoNative_HmacUpdate(jobject ctx, uint8_t* data, int32_t len)
 {
-    if (!ctx)
+    if (!ctx || (len > 0 && !data))
         return FAIL;
 
-    abort_if_invalid_pointer_argument (data);
+    if (len == 0)
+        return SUCCESS;
+
     JNIEnv* env = GetJNIEnv();
     jbyteArray dataBytes = make_java_byte_array(env, len);
     (*env)->SetByteArrayRegion(env, dataBytes, 0, len, (jbyte*)data);

Explanations:

An empty source makes sense for the hash one-shot iif the length is zero. Fully asserting it is not null doesn't make sense, it can be null if the length is zero.

For the HmacUpdate, we can just return SUCCESS if the length is zero. There is no work to do in that case (and makes for a nice little optimization). We should check though that the data is not a null pointer if the data length is greater than zero.

[bartonjs]

Personally, I'd just put an if in the managed code to avoid calling the P/Invoke if the length is zero. Saves on all the pinning and calling work.

[vcsjones]

I can do that, too. @AaronRobinsonMSFT would you prefer I open a PR to replace this one? Happy to take it off your hands.

[AaronRobinsonMSFT]

@vcsjones I will take you up on that offer. Thanks.