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.

Sign up for GitHub

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

Jump to bottom

cmd/cgo: add JNI's jmethodID/jfieldID to uintptr for Android NDK #40955

Closed
wants to merge 1 commit into from
Closed

cmd/cgo: add JNI's jmethodID/jfieldID to uintptr for Android NDK #40955

wants to merge 1 commit into from

Conversation

steeve
Copy link
Contributor

@steeve steeve commented Aug 21, 2020

In Android's NDK, jmethodID and jfieldID are defined as:

struct _jfieldID;                       /* opaque structure */
typedef struct _jfieldID* jfieldID;     /* field IDs */
struct _jmethodID;                      /* opaque structure */
typedef struct _jmethodID* jmethodID;   /* method IDs */

Since Go seems them as pointers, a crash can occur at runtime if the GC
tries to dereference those pointers. This is very reproducible in
Android 11 for some reason.

Add those two types to the badJNI types and rework the check logic a
bit.

Fixes #40954

@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Aug 21, 2020
@steeve
Copy link
Contributor Author

steeve commented Aug 21, 2020

Adding the test in a few minutes

@gopherbot
Copy link

This PR (HEAD: 752d04f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/249744 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://golang.org/doc/contribute.html#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://golang.org/s/release
for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/249744.
After addressing review feedback, remember to publish your drafts!

@steeve
cmd/cgo: add JNI's jmethodID/jfieldID to uintptr for Android NDK
fd17877 
In Android's NDK, jmethodID and jfieldID are defined as:

	struct _jfieldID;                       /* opaque structure */
	typedef struct _jfieldID* jfieldID;     /* field IDs */
	struct _jmethodID;                      /* opaque structure */
	typedef struct _jmethodID* jmethodID;   /* method IDs */

Since Go seems them as pointers, a crash can occur at runtime if the GC
tries to dereference those pointers. This is very reproducible in
Android 11 for some reason.

Add those two types to the badJNI types and rework the check logic a
bit.

Fixes #40954

Signed-off-by: Steeve Morin <steeve@zen.ly>
@gopherbot
Copy link

This PR (HEAD: fd17877) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/249744 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link

Message from Emmanuel Odeke:

Patch Set 2: Run-TryBot+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/249744.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gobot Gobot:

Patch Set 2:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=d25c3654

Please don’t reply on this GitHub thread. Visit golang.org/cl/249744.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gobot Gobot:

Patch Set 2: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/249744.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Steeve Morin:

Patch Set 2:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/249744.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Keith Randall:

Patch Set 2:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/249744.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Emmanuel Odeke:

Patch Set 2:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/249744.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Go Bot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://golang.org/doc/contribute.html#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://golang.org/s/release
for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/249744.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Go Bot:

Patch Set 2:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=d25c3654

Please don’t reply on this GitHub thread. Visit golang.org/cl/249744.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Go Bot:

Patch Set 2: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/249744.
After addressing review feedback, remember to publish your drafts!

@steeve
Copy link
Contributor Author

steeve commented Nov 13, 2021

Hey all. I think we can merge this. I'm just coming back to it (we were running on a fork).
We've been running it for over a year now, and I just found out it wasn't merged by getting the error or 1.17.2.

@ianlancetaylor
Copy link
Contributor

@steeve Note that few people see comments on GitHub pull requests. The best place to comment is in Gerritt, in this case at https://golang.org/cl/249744.

That said, the expectation here was that this was fixed by https://golang.org/cl/250940, and that this particular pull request is not needed. The associated issue #40954 is closed as fixed. If that issue is not fixed, please reply on that issue, or open a new one. Thanks.

@gopherbot
Copy link

This PR is being closed because golang.org/cl/249744 has been abandoned.

We think that the problem this patch addresses was fixed by https://golang.org/cl/250940. Please comment here or at https://golang.org/issue/40954 if you don't agree. Thanks.

@gopherbot gopherbot closed this Nov 13, 2021
@steeve
Copy link
Contributor Author

steeve commented Nov 14, 2021

Indeed! Sorry for the noise!

@steeve
Copy link
Contributor Author

steeve commented Nov 21, 2021

I'm sorry to report that I got a crash on vanilla Go 1.17.2 when trying to manipulate jmethodID and jfieldID:

11-21 01:35:39.090 31051     0 E Go      : runtime: bad pointer in frame java/android/content.Context.TryGetSystemService at 0x4001c34d08: 0x97
11-21 01:35:39.090 31051     0 E Go      : fatal error: invalid pointer found on stack
11-21 01:35:39.097 31051     0 E Go      : 
11-21 01:35:39.097 31051     0 E Go      : runtime stack:
11-21 01:35:39.098 31051     0 E Go      : runtime.throw({0x78447e742b, 0x1e})
11-21 01:35:39.098 31051     0 E Go      : 	GOROOT/src/runtime/panic.go:1198 +0x54 fp=0x781146e380 sp=0x781146e350 pc=0x7844c07e94
11-21 01:35:39.098 31051     0 E Go      : runtime.adjustpointers(0x4001dc3d00, 0x781146e470, 0x781146e878, {0x7847216e68, 0x7847596cc0})
11-21 01:35:39.098 31051     0 E Go      : 	GOROOT/src/runtime/stack.go:617 +0x210 fp=0x781146e3d0 sp=0x781146e380 pc=0x7844c22fc0
11-21 01:35:39.098 31051     0 E Go      : runtime.adjustframe(0x781146e790, 0x781146e878)
11-21 01:35:39.098 31051     0 E Go      : 	GOROOT/src/runtime/stack.go:659 +0xd4 fp=0x781146e4b0 sp=0x781146e3d0 pc=0x7844c230a4
11-21 01:35:39.098 31051     0 E Go      : runtime.gentraceback(0xffffffffffffffff, 0xffffffffffffffff, 0x0, 0x4000aa4000, 0x0, 0x0, 0x7fffffff, 0x7846548b68, 0x781146e878, 0x0)
11-21 01:35:39.098 31051     0 E Go      : 	GOROOT/src/runtime/traceback.go:350 +0x780 fp=0x781146e7f0 sp=0x781146e4b0 pc=0x7844c2fae0
11-21 01:35:39.098 31051     0 E Go      : runtime.copystack(0x4000aa4000, 0x1000)
11-21 01:35:39.098 31051     0 E Go      : 	GOROOT/src/runtime/stack.go:918 +0x264 fp=0x781146e9a0 sp=0x781146e7f0 pc=0x7844c23824
11-21 01:35:39.098 31051     0 E Go      : runtime.newstack()
11-21 01:35:39.098 31051     0 E Go      : 	GOROOT/src/runtime/stack.go:1097 +0x428 fp=0x781146eb50 sp=0x781146e9a0 pc=0x7844c23d38
11-21 01:35:39.098 31051     0 E Go      : runtime.morestack()
11-21 01:35:39.098 31051     0 E Go      : 	src/runtime/asm_arm64.s:303 +0x70 fp=0x781146eb50 sp=0x781146eb50 pc=0x7844c3bbc0

They are indeed not in the heap, but it seems since they are opaque types disguised as pointer, some check still try to figure out if they are valid (they are not).

With this PR applied, the crash is fixed, however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cmd/cgo: jmethodID/jfieldID is not mapped to uintptr if building with the Android NDK
4 participants
@steeve @gopherbot @ianlancetaylor @googlebot