Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Mon, 4 Dec 2023 23:32:52 GMT, David Holmes wrote: > The skara tooling does not currently support our rules but it remains as > always that non-trivial Hotspot changes require two reviewers. Thanks, I will keep this in mind. And I apologise for not following the process, though not intentionally. - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1842611183
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Wed, 29 Nov 2023 11:49:31 GMT, Jaroslav Bachorik wrote: >> Please, review this fix for a corner case handling of `jmethodID` values. >> >> The issue is related to the interplay between `jmethodID` values and method >> redefinitions. Each `jmethodID` value is effectively a pointer to a `Method` >> instance. Once that method gets redefined, the `jmethodID` is updated to >> point to the last `Method` version. >> Unless the method is still on stack/running, in which case the original >> `jmethodID` will be redirected to the latest `Method` version and at the >> same time the 'previous' `Method` version will receive a new `jmethodID` >> pointing to that previous version. >> >> If we happen to capture stacktrace via `GetStackTrace` or >> `GetAllStackTraces` JVMTI calls while this previous `Method` version is >> still on stack we will have the corresponding frame identified by a >> `jmethodID` pointing to that version. >> However, sooner or later the 'previous' class version becomes eligible for >> cleanup at what time all contained `Method` instances. The cleanup process >> will not perform the `jmethodID` pointer maintenance and we will end up with >> pointers to deallocated memory. >> This is caused by the fact that the `jmethodID` lifecycle is bound to >> `ClassLoaderData` instance and all relevant `jmethodID`s will get >> batch-updated when the class loader is being released and all its classes >> are getting unloaded. >> >> This means that we need to make sure that if a `Method` instance is being >> deallocate the associated `jmethodID` (if any) must not point to the >> deallocated instance once we are finished. Unfortunately, we can not just >> update the `jmethodID` values in bulk when purging an old class version - >> the per `InstanceKlass` jmethodID cache is present only for the main class >> version and contains `jmethodID` values for both the old and current method >> versions. >> >> ~Therefore we need to perform `jmethodID` lookup when we are about to >> deallocate a `Method` instance and clean up the pointer only if that >> `jmethodID` is pointing to the `Method` instance which is being deallocated.~ >> >> Therefore, we need to perform `jmethodID` lookup for each method in an old >> class version that is getting purged, and null out the pointer of that >> `jmethodID` to break the link from `jmethodID` to the method instance that >> is about to get deallocated. >> >> _(For anyone interested, a much lengthier writeup is available in [my >> blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_ > > Jaroslav Bachorik has updated the pull request incrementally with one > additional commit since the last revision: > > Restrict cleanup to obsolete methods only Just for the record Coleen's review is marked as "Review applies to [81e31dae](https://git.openjdk.org/jdk/pull/16662/files/81e31daeef4c68352368a90e3ab453ba9b33650c)" - which is not the final version. The skara tooling does not currently support our rules but it remains as always that non-trivial Hotspot changes require two reviewers. Thanks for the additional explanations Coleen. - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1839721631
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Mon, 4 Dec 2023 11:11:27 GMT, Thomas Stuefe wrote: > Okay so why does this happen and is it a reasonable thing to be happening? On > the surface it sounds wrong to deallocate anything associated with a live > classloader. If we didn't deallocate these old methods, there would be a memory leak when using class redefinition. It would be a lot simpler if we didn't have to do this though. - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1838543829
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Wed, 29 Nov 2023 11:49:31 GMT, Jaroslav Bachorik wrote: >> Please, review this fix for a corner case handling of `jmethodID` values. >> >> The issue is related to the interplay between `jmethodID` values and method >> redefinitions. Each `jmethodID` value is effectively a pointer to a `Method` >> instance. Once that method gets redefined, the `jmethodID` is updated to >> point to the last `Method` version. >> Unless the method is still on stack/running, in which case the original >> `jmethodID` will be redirected to the latest `Method` version and at the >> same time the 'previous' `Method` version will receive a new `jmethodID` >> pointing to that previous version. >> >> If we happen to capture stacktrace via `GetStackTrace` or >> `GetAllStackTraces` JVMTI calls while this previous `Method` version is >> still on stack we will have the corresponding frame identified by a >> `jmethodID` pointing to that version. >> However, sooner or later the 'previous' class version becomes eligible for >> cleanup at what time all contained `Method` instances. The cleanup process >> will not perform the `jmethodID` pointer maintenance and we will end up with >> pointers to deallocated memory. >> This is caused by the fact that the `jmethodID` lifecycle is bound to >> `ClassLoaderData` instance and all relevant `jmethodID`s will get >> batch-updated when the class loader is being released and all its classes >> are getting unloaded. >> >> This means that we need to make sure that if a `Method` instance is being >> deallocate the associated `jmethodID` (if any) must not point to the >> deallocated instance once we are finished. Unfortunately, we can not just >> update the `jmethodID` values in bulk when purging an old class version - >> the per `InstanceKlass` jmethodID cache is present only for the main class >> version and contains `jmethodID` values for both the old and current method >> versions. >> >> ~Therefore we need to perform `jmethodID` lookup when we are about to >> deallocate a `Method` instance and clean up the pointer only if that >> `jmethodID` is pointing to the `Method` instance which is being deallocated.~ >> >> Therefore, we need to perform `jmethodID` lookup for each method in an old >> class version that is getting purged, and null out the pointer of that >> `jmethodID` to break the link from `jmethodID` to the method instance that >> is about to get deallocated. >> >> _(For anyone interested, a much lengthier writeup is available in [my >> blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_ > > Jaroslav Bachorik has updated the pull request incrementally with one > additional commit since the last revision: > > Restrict cleanup to obsolete methods only We deallocate the old Method* if nothing is referring to them (ie they're not running or being referenced for some other reason). Look at MetadataOnStackMark. The jmethodIDs to an obsolete method were a dangling pointer and we want to just null them out. The old Method* are attached to the scratch_version of the InstanceKlass so we essentially remove that and walk down to the Methods when none of them are found. - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1838538568
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Mon, 4 Dec 2023 00:41:23 GMT, David Holmes wrote: > From the blog: > > > Yes! The methods are being deallocated for a class loader that is still > > alive. > > Okay so why does this happen and is it a reasonable thing to be happening? On > the surface it sounds wrong to deallocate anything associated with a live > classloader. This sounds odd to me to. I know that we deallocate the old *byte code* of re-transformed classes; but `Method*` ? - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1838413238
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Fri, 1 Dec 2023 05:15:15 GMT, Thomas Stuefe wrote: >> Jaroslav Bachorik has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Restrict cleanup to obsolete methods only > > I won't be able to review this this week, too snowed in atm. I can take a > look next week. We can always just revert the change if needed. > > Thinking about Skara, I think as long as we have this confusing mixture of > rules (hotspot wants 2 reviewers that are Reviewer/Committer, but some jdk > libs only want one, but then you need two for desktop I think otherwise Phil > gets angry) - we should hard-code the 2-reviewer rule into skara as default > since it affects the lion's share of all changes. @tstuefe I got confused by the Skara tooling. I had a vague memory of some discussions going on about relaxing the requirement of 2 reviewers for some parts to the code base and I thought I was in a good shape seeing the Skara checkbox.  As for not having a review for the final version - I am not that restless. I specifically dismissed the previous review to avoid incidentally integrating based on a review of a version that was not actual. Then I asked @coleenp to re-do the review on the final bits (https://github.com/openjdk/jdk/pull/16662#issuecomment-1827432032) @dholmes-ora >Okay so why does this happen and is it a reasonable thing to be happening? On >the surface it sounds wrong to deallocate anything associated with a live >classloader. This is happening for previous versions of retransformed methods. As long as those methods are still on stack they are kept alive. But once they are not executing they are free to be destroyed. And this is where the problem was happening - the previous versions of methods were being destroyed but the associated jmethodIDs were not updated not to point to what became an invalid memory block. - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1838221097
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Wed, 29 Nov 2023 11:49:31 GMT, Jaroslav Bachorik wrote: >> Please, review this fix for a corner case handling of `jmethodID` values. >> >> The issue is related to the interplay between `jmethodID` values and method >> redefinitions. Each `jmethodID` value is effectively a pointer to a `Method` >> instance. Once that method gets redefined, the `jmethodID` is updated to >> point to the last `Method` version. >> Unless the method is still on stack/running, in which case the original >> `jmethodID` will be redirected to the latest `Method` version and at the >> same time the 'previous' `Method` version will receive a new `jmethodID` >> pointing to that previous version. >> >> If we happen to capture stacktrace via `GetStackTrace` or >> `GetAllStackTraces` JVMTI calls while this previous `Method` version is >> still on stack we will have the corresponding frame identified by a >> `jmethodID` pointing to that version. >> However, sooner or later the 'previous' class version becomes eligible for >> cleanup at what time all contained `Method` instances. The cleanup process >> will not perform the `jmethodID` pointer maintenance and we will end up with >> pointers to deallocated memory. >> This is caused by the fact that the `jmethodID` lifecycle is bound to >> `ClassLoaderData` instance and all relevant `jmethodID`s will get >> batch-updated when the class loader is being released and all its classes >> are getting unloaded. >> >> This means that we need to make sure that if a `Method` instance is being >> deallocate the associated `jmethodID` (if any) must not point to the >> deallocated instance once we are finished. Unfortunately, we can not just >> update the `jmethodID` values in bulk when purging an old class version - >> the per `InstanceKlass` jmethodID cache is present only for the main class >> version and contains `jmethodID` values for both the old and current method >> versions. >> >> ~Therefore we need to perform `jmethodID` lookup when we are about to >> deallocate a `Method` instance and clean up the pointer only if that >> `jmethodID` is pointing to the `Method` instance which is being deallocated.~ >> >> Therefore, we need to perform `jmethodID` lookup for each method in an old >> class version that is getting purged, and null out the pointer of that >> `jmethodID` to break the link from `jmethodID` to the method instance that >> is about to get deallocated. >> >> _(For anyone interested, a much lengthier writeup is available in [my >> blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_ > > Jaroslav Bachorik has updated the pull request incrementally with one > additional commit since the last revision: > > Restrict cleanup to obsolete methods only >From the blog: > Yes! The methods are being deallocated for a class loader that is still alive. Okay so why does this happen and is it a reasonable thing to be happening? On the surface it sounds wrong to deallocate anything associated with a live classloader. - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1837668001
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Wed, 29 Nov 2023 11:49:31 GMT, Jaroslav Bachorik wrote: >> Please, review this fix for a corner case handling of `jmethodID` values. >> >> The issue is related to the interplay between `jmethodID` values and method >> redefinitions. Each `jmethodID` value is effectively a pointer to a `Method` >> instance. Once that method gets redefined, the `jmethodID` is updated to >> point to the last `Method` version. >> Unless the method is still on stack/running, in which case the original >> `jmethodID` will be redirected to the latest `Method` version and at the >> same time the 'previous' `Method` version will receive a new `jmethodID` >> pointing to that previous version. >> >> If we happen to capture stacktrace via `GetStackTrace` or >> `GetAllStackTraces` JVMTI calls while this previous `Method` version is >> still on stack we will have the corresponding frame identified by a >> `jmethodID` pointing to that version. >> However, sooner or later the 'previous' class version becomes eligible for >> cleanup at what time all contained `Method` instances. The cleanup process >> will not perform the `jmethodID` pointer maintenance and we will end up with >> pointers to deallocated memory. >> This is caused by the fact that the `jmethodID` lifecycle is bound to >> `ClassLoaderData` instance and all relevant `jmethodID`s will get >> batch-updated when the class loader is being released and all its classes >> are getting unloaded. >> >> This means that we need to make sure that if a `Method` instance is being >> deallocate the associated `jmethodID` (if any) must not point to the >> deallocated instance once we are finished. Unfortunately, we can not just >> update the `jmethodID` values in bulk when purging an old class version - >> the per `InstanceKlass` jmethodID cache is present only for the main class >> version and contains `jmethodID` values for both the old and current method >> versions. >> >> ~Therefore we need to perform `jmethodID` lookup when we are about to >> deallocate a `Method` instance and clean up the pointer only if that >> `jmethodID` is pointing to the `Method` instance which is being deallocated.~ >> >> Therefore, we need to perform `jmethodID` lookup for each method in an old >> class version that is getting purged, and null out the pointer of that >> `jmethodID` to break the link from `jmethodID` to the method instance that >> is about to get deallocated. >> >> _(For anyone interested, a much lengthier writeup is available in [my >> blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_ > > Jaroslav Bachorik has updated the pull request incrementally with one > additional commit since the last revision: > > Restrict cleanup to obsolete methods only I won't be able to review this this week, too snowed in atm. I can take a look next week. We can always just revert the change if needed. Thinking about Skara, I think as long as we have this confusing mixture of rules (hotspot wants 2 reviewers that are Reviewer/Committer, but some jdk libs only want one, but then you need two for desktop I think otherwise Phil gets angry) - we should hard-code the 2-reviewer rule into skara as default since it affects the lion's share of all changes. - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1835474161
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Thu, 30 Nov 2023 06:06:48 GMT, David Holmes wrote: >> Thanks everyone involved in reviewing this PR! You were awesome and helped >> me drive the PR to better place than it started! > > @jbachorik this should not have been integrated yet! You only have one > review not the required two for hotspot changes. Further your one Reviewer > didn't even review the final version of the change! I did review the final version of the change. Can @dholmes-ora or @tstuefe review again and we'll open CRs for anything missed? - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1833758619
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Wed, 29 Nov 2023 17:27:30 GMT, Jaroslav Bachorik wrote: >> Jaroslav Bachorik has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Restrict cleanup to obsolete methods only > > Thanks everyone involved in reviewing this PR! You were awesome and helped me > drive the PR to better place than it started! @jbachorik this should not have been integrated yet! You only have one review not the required two for hotspot changes. Further your one Reviewer didn't even review the final version of the change! - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1833162015
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Wed, 29 Nov 2023 11:49:31 GMT, Jaroslav Bachorik wrote: >> Please, review this fix for a corner case handling of `jmethodID` values. >> >> The issue is related to the interplay between `jmethodID` values and method >> redefinitions. Each `jmethodID` value is effectively a pointer to a `Method` >> instance. Once that method gets redefined, the `jmethodID` is updated to >> point to the last `Method` version. >> Unless the method is still on stack/running, in which case the original >> `jmethodID` will be redirected to the latest `Method` version and at the >> same time the 'previous' `Method` version will receive a new `jmethodID` >> pointing to that previous version. >> >> If we happen to capture stacktrace via `GetStackTrace` or >> `GetAllStackTraces` JVMTI calls while this previous `Method` version is >> still on stack we will have the corresponding frame identified by a >> `jmethodID` pointing to that version. >> However, sooner or later the 'previous' class version becomes eligible for >> cleanup at what time all contained `Method` instances. The cleanup process >> will not perform the `jmethodID` pointer maintenance and we will end up with >> pointers to deallocated memory. >> This is caused by the fact that the `jmethodID` lifecycle is bound to >> `ClassLoaderData` instance and all relevant `jmethodID`s will get >> batch-updated when the class loader is being released and all its classes >> are getting unloaded. >> >> This means that we need to make sure that if a `Method` instance is being >> deallocate the associated `jmethodID` (if any) must not point to the >> deallocated instance once we are finished. Unfortunately, we can not just >> update the `jmethodID` values in bulk when purging an old class version - >> the per `InstanceKlass` jmethodID cache is present only for the main class >> version and contains `jmethodID` values for both the old and current method >> versions. >> >> ~Therefore we need to perform `jmethodID` lookup when we are about to >> deallocate a `Method` instance and clean up the pointer only if that >> `jmethodID` is pointing to the `Method` instance which is being deallocated.~ >> >> Therefore, we need to perform `jmethodID` lookup for each method in an old >> class version that is getting purged, and null out the pointer of that >> `jmethodID` to break the link from `jmethodID` to the method instance that >> is about to get deallocated. >> >> _(For anyone interested, a much lengthier writeup is available in [my >> blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_ > > Jaroslav Bachorik has updated the pull request incrementally with one > additional commit since the last revision: > > Restrict cleanup to obsolete methods only Thanks everyone involved in reviewing this PR! You were awesome and helped me drive the PR to better place than it started! - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1832386439
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
> Please, review this fix for a corner case handling of `jmethodID` values. > > The issue is related to the interplay between `jmethodID` values and method > redefinitions. Each `jmethodID` value is effectively a pointer to a `Method` > instance. Once that method gets redefined, the `jmethodID` is updated to > point to the last `Method` version. > Unless the method is still on stack/running, in which case the original > `jmethodID` will be redirected to the latest `Method` version and at the same > time the 'previous' `Method` version will receive a new `jmethodID` pointing > to that previous version. > > If we happen to capture stacktrace via `GetStackTrace` or `GetAllStackTraces` > JVMTI calls while this previous `Method` version is still on stack we will > have the corresponding frame identified by a `jmethodID` pointing to that > version. > However, sooner or later the 'previous' class version becomes eligible for > cleanup at what time all contained `Method` instances. The cleanup process > will not perform the `jmethodID` pointer maintenance and we will end up with > pointers to deallocated memory. > This is caused by the fact that the `jmethodID` lifecycle is bound to > `ClassLoaderData` instance and all relevant `jmethodID`s will get > batch-updated when the class loader is being released and all its classes are > getting unloaded. > > This means that we need to make sure that if a `Method` instance is being > deallocate the associated `jmethodID` (if any) must not point to the > deallocated instance once we are finished. Unfortunately, we can not just > update the `jmethodID` values in bulk when purging an old class version - the > per `InstanceKlass` jmethodID cache is present only for the main class > version and contains `jmethodID` values for both the old and current method > versions. > > ~Therefore we need to perform `jmethodID` lookup when we are about to > deallocate a `Method` instance and clean up the pointer only if that > `jmethodID` is pointing to the `Method` instance which is being deallocated.~ > > Therefore, we need to perform `jmethodID` lookup for each method in an old > class version that is getting purged, and null out the pointer of that > `jmethodID` to break the link from `jmethodID` to the method instance that is > about to get deallocated. > > _(For anyone interested, a much lengthier writeup is available in [my > blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_ Jaroslav Bachorik has updated the pull request incrementally with one additional commit since the last revision: Restrict cleanup to obsolete methods only - Changes: - all: https://git.openjdk.org/jdk/pull/16662/files - new: https://git.openjdk.org/jdk/pull/16662/files/81e31dae..aae367fb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16662=10 - incr: https://webrevs.openjdk.org/?repo=jdk=16662=09-10 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16662.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16662/head:pull/16662 PR: https://git.openjdk.org/jdk/pull/16662
