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

[Java.Interop] RegisterNatives() shouldn't exceed numMethods #1029

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

jonpryor
Copy link
Member

Context: dotnet/android#7285 (comment)

Java.Inteorp.JniEnvironment.Types.RegisterNatives() is overloaded:

namespace Java.Interop {
  partial class JniEnvironment {
    partial class Types {
      public static void RegisterNatives (JniObjectReference type, JniNativeMethodRegistration[] methods);
      public static void RegisterNatives (JniObjectReference type, JniNativeMethodRegistration[] methods, int numMethods);
    }
  }
}

If the int numMethods overload is used, then:

  1. it should be possible to pass an array which contains more than
    numMethods elements, and
  2. Passing such an array shouldn't throw an exception.

For example:

var methods = new JniNativeMethodRegistration [10];
JniEnvironment.Types.RegisterNatives (type, methods, 0);

Instead, when using a Debug configuration build of Java.Interop.dll,
a NullReferenceException would be thrown, because the foreach
loop would traverse every element, not just the first numMethods
elements, which could result in accessing
methods[i].Marshaler.GetType(), which could be null.

Fix the DEBUG && NETCOREAPP check so that only the first
numMethods elements are accessed, not all of them, and add a
null check around JniNativeMethodRegistration.Marshaler access.

This prevents the NullReferenceException.

@jonpryor jonpryor force-pushed the jonp-fix-registernatives-numMethods branch from bdc2053 to 6749463 Compare August 23, 2022 18:16
`Java.Inteorp.JniEnvironment.Types.RegisterNatives()` is overloaded:

	namespace Java.Interop {
	  partial class JniEnvironment {
	    partial class Types {
	      public static void RegisterNatives (JniObjectReference type, JniNativeMethodRegistration[] methods);
	      public static void RegisterNatives (JniObjectReference type, JniNativeMethodRegistration[] methods, int numMethods);
	    }
	  }
	}

If the `int numMethods` overload is used, then:

 1. it should be possible to pass an array which contains *more* than
    `numMethods` elements, and
 2. Passing such an array shouldn't throw an exception.

For example:

	var methods = new JniNativeMethodRegistration [10];
	JniEnvironment.Types.RegisterNatives (type, methods, 0);

Instead, when using a Debug configuration build of `Java.Interop.dll`,
a `NullReferenceException` would be thrown, because the `foreach`
loop would traverse *every element*, not just the first `numMethods`
elements, which could result in accessing
`methods[i].Marshaler.GetType()`, which could be `null`.

Fix the `DEBUG && NETCOREAPP` check so that only the first
`numMethods` elements are accessed, *not* all of them, and add a
`null` check around `JniNativeMethodRegistration.Marshaler` access.

This prevents the `NullReferenceException`.

Additionally, add some parameter validation and throw an
`ArgumentOutOfRangeException` if `numMethods` is negative or is
greater than `methods.Length`.
@jonpryor jonpryor force-pushed the jonp-fix-registernatives-numMethods branch from 6749463 to 42e652a Compare August 23, 2022 18:50
@jonpryor jonpryor merged commit e31d9c6 into dotnet:main Aug 23, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants