Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
On 9/15/16 19:13, David Holmes wrote:
On 16/09/2016 8:52 AM, serguei.spit...@oracle.com wrote:
Hi Harold,
I did not got deep into the fix yet but wonder why the JVMTI function is
My copy-paste failed.
I wanted to list the JVMTI function name: GetMethodDeclaringClass.
Thanks,
Serguei
not used.
I was wondering a similar thing. It seems very heavyweight to use Java
level reflection from inside native code to validate the native
"handles" passed to that native code. I would have expected a way to
go from a MethodId to the declaring class of the method, and a simple
way to test if there is an ancestor relation between the two classes.
On 9/15/16 13:05, harold seigel wrote:
One could argue that a spec compliant JNI implementation wouldn't need
this change in the first place...
Regardless, I'm withdrawing this change because I found that it fails
a com/sun/jdi JTreg test involving static methods in interfaces.
I find it both intriguing and worrying that ClassType.InvokeMethod
refers to superinterfaces when prior to 8 (and this spec was not
updated in this area) static interface methods did not exist! The main
changes were in the definition of InterfaceType.InvokeMethod. I wonder
whether invocation of static interface methods via
ClassType.InvokeMethod is actually tested directly?
I realize the specs are a bit of a minefield when it comes to what is
required by the different specs and what is implemented in hotspot.
Unfortunately it is a minefield I also have to wade through for
private interface methods. In many cases it is not clear what should
happen and all we have to guide us is what hotspot does (eg "virtual"
invocations on non-virtual methods).
David
-
Thanks, Harold
On 9/15/2016 3:37 PM, Daniel D. Daugherty wrote:
On 9/15/16 12:10 PM, harold seigel wrote:
(Adding hotspot-runtime)
Hi Dan,
Thanks for looking at this.
I could pass NULL instead of clazz to ToReflectMethod() to ensure
that the method object isn't being obtained from clazz.
I don't think that would be a JNI spec compliant use of the
JNI ToReflectedMethod() function. That would be relying on
the fact that HotSpot doesn't use the clazz parameter to
convert {clazz,jmethodID} => method_object.
Sorry... again...
Dan
Harold
On 9/15/2016 1:09 PM, Daniel D. Daugherty wrote:
On 9/15/16 9:31 AM, harold seigel wrote:
Hi,
Please review this small fix for JDK-8160987. The JDWP
InvokeStatic() method was depending on the JNI function that it
called to enforce the requirement that the specified method must
be a member of the specified class or one of its super classes.
But, JNI does not enforce this requirement. This fix adds code to
JDWP to do its own check that the specified method is a member of
the specified class or one of its super classes.
JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8160987
Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8160987/
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
Sorry I didn't think of this comment during the pre-review...
The only "strange" part of this fix is:
L374: /* Get the method object from the method's
jmethodID. */
L375: method_object =
JNI_FUNC_PTR(env,ToReflectedMethod)(env,
L376: clazz,
L377: method,
L378: JNI_TRUE /* isStatic */);
L379: if (method_object == NULL) {
L380: return JVMTI_ERROR_NONE; /* Bad jmethodID ? This
will be handled elsewhere */
L381: }
Here we are using parameter 'clazz' to find the method_object
for
parameter 'method' so that we can validate that 'clazz'
refers to
method's class or superclass.
When a bogus 'clazz' value is passed in by a JCK test, the only
reason that JNI ToReflectedMethod() can still find the right
method_object is that our (HotSpot) implementation of JNI
ToReflectedMethod() doesn't really require the 'clazz' parameter
to find the right method_object. So the 'method_object' that we
return is the real one which has a 'clazz' field that doesn't
match the 'clazz' parameter.
Wow does that twist your head around or what?
So we're trusting JNI ToReflectedMethod() to return the right
method_object when we give it a potentially bad 'clazz' value.
So should we use JNI FromReflectedMethod() to convert the
method_object back into a jmethodID and verify that jmethodID
matches the one that we passed to check_methodClass()?
I might be too paranoid here so feel free to say that enough is
enough with this fix.
Thumbs up!
Dan
The fix was tested with the two failing JCK vm/jdwp tests listed
in the bug, the JCK Lang, VM, and API tests, the hotspot JTReg
tests, the java/lang, java/util and other JTReg tests, the
co-located and non-colocated NSK tests, and with the RBT Tier2
tests.
Thanks, Harold
Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
On 16/09/2016 8:52 AM, serguei.spit...@oracle.com wrote:
Hi Harold,
I did not got deep into the fix yet but wonder why the JVMTI function is
not used.
I was wondering a similar thing. It seems very heavyweight to use Java
level reflection from inside native code to validate the native
"handles" passed to that native code. I would have expected a way to go
from a MethodId to the declaring class of the method, and a simple way
to test if there is an ancestor relation between the two classes.
On 9/15/16 13:05, harold seigel wrote:
One could argue that a spec compliant JNI implementation wouldn't need
this change in the first place...
Regardless, I'm withdrawing this change because I found that it fails
a com/sun/jdi JTreg test involving static methods in interfaces.
I find it both intriguing and worrying that ClassType.InvokeMethod
refers to superinterfaces when prior to 8 (and this spec was not updated
in this area) static interface methods did not exist! The main changes
were in the definition of InterfaceType.InvokeMethod. I wonder whether
invocation of static interface methods via ClassType.InvokeMethod is
actually tested directly?
I realize the specs are a bit of a minefield when it comes to what is
required by the different specs and what is implemented in hotspot.
Unfortunately it is a minefield I also have to wade through for private
interface methods. In many cases it is not clear what should happen and
all we have to guide us is what hotspot does (eg "virtual" invocations
on non-virtual methods).
David
-
Thanks, Harold
On 9/15/2016 3:37 PM, Daniel D. Daugherty wrote:
On 9/15/16 12:10 PM, harold seigel wrote:
(Adding hotspot-runtime)
Hi Dan,
Thanks for looking at this.
I could pass NULL instead of clazz to ToReflectMethod() to ensure
that the method object isn't being obtained from clazz.
I don't think that would be a JNI spec compliant use of the
JNI ToReflectedMethod() function. That would be relying on
the fact that HotSpot doesn't use the clazz parameter to
convert {clazz,jmethodID} => method_object.
Sorry... again...
Dan
Harold
On 9/15/2016 1:09 PM, Daniel D. Daugherty wrote:
On 9/15/16 9:31 AM, harold seigel wrote:
Hi,
Please review this small fix for JDK-8160987. The JDWP
InvokeStatic() method was depending on the JNI function that it
called to enforce the requirement that the specified method must
be a member of the specified class or one of its super classes.
But, JNI does not enforce this requirement. This fix adds code to
JDWP to do its own check that the specified method is a member of
the specified class or one of its super classes.
JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8160987
Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8160987/
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
Sorry I didn't think of this comment during the pre-review...
The only "strange" part of this fix is:
L374: /* Get the method object from the method's jmethodID. */
L375: method_object = JNI_FUNC_PTR(env,ToReflectedMethod)(env,
L376: clazz,
L377: method,
L378: JNI_TRUE /* isStatic */);
L379: if (method_object == NULL) {
L380: return JVMTI_ERROR_NONE; /* Bad jmethodID ? This
will be handled elsewhere */
L381: }
Here we are using parameter 'clazz' to find the method_object for
parameter 'method' so that we can validate that 'clazz' refers to
method's class or superclass.
When a bogus 'clazz' value is passed in by a JCK test, the only
reason that JNI ToReflectedMethod() can still find the right
method_object is that our (HotSpot) implementation of JNI
ToReflectedMethod() doesn't really require the 'clazz' parameter
to find the right method_object. So the 'method_object' that we
return is the real one which has a 'clazz' field that doesn't
match the 'clazz' parameter.
Wow does that twist your head around or what?
So we're trusting JNI ToReflectedMethod() to return the right
method_object when we give it a potentially bad 'clazz' value.
So should we use JNI FromReflectedMethod() to convert the
method_object back into a jmethodID and verify that jmethodID
matches the one that we passed to check_methodClass()?
I might be too paranoid here so feel free to say that enough is
enough with this fix.
Thumbs up!
Dan
The fix was tested with the two failing JCK vm/jdwp tests listed
in the bug, the JCK Lang, VM, and API tests, the hotspot JTReg
tests, the java/lang, java/util and other JTReg tests, the
co-located and non-colocated NSK tests, and with the RBT Tier2 tests.
Thanks, Harold
Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
Hi Harold,
I did not got deep into the fix yet but wonder why the JVMTI function is
not used.
Thanks,
Serguei
On 9/15/16 13:05, harold seigel wrote:
Hi Dan,
One could argue that a spec compliant JNI implementation wouldn't need
this change in the first place...
Regardless, I'm withdrawing this change because I found that it fails
a com/sun/jdi JTreg test involving static methods in interfaces.
Thanks, Harold
On 9/15/2016 3:37 PM, Daniel D. Daugherty wrote:
On 9/15/16 12:10 PM, harold seigel wrote:
(Adding hotspot-runtime)
Hi Dan,
Thanks for looking at this.
I could pass NULL instead of clazz to ToReflectMethod() to ensure
that the method object isn't being obtained from clazz.
I don't think that would be a JNI spec compliant use of the
JNI ToReflectedMethod() function. That would be relying on
the fact that HotSpot doesn't use the clazz parameter to
convert {clazz,jmethodID} => method_object.
Sorry... again...
Dan
Harold
On 9/15/2016 1:09 PM, Daniel D. Daugherty wrote:
On 9/15/16 9:31 AM, harold seigel wrote:
Hi,
Please review this small fix for JDK-8160987. The JDWP
InvokeStatic() method was depending on the JNI function that it
called to enforce the requirement that the specified method must
be a member of the specified class or one of its super classes.
But, JNI does not enforce this requirement. This fix adds code to
JDWP to do its own check that the specified method is a member of
the specified class or one of its super classes.
JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8160987
Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8160987/
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
Sorry I didn't think of this comment during the pre-review...
The only "strange" part of this fix is:
L374: /* Get the method object from the method's jmethodID. */
L375: method_object = JNI_FUNC_PTR(env,ToReflectedMethod)(env,
L376: clazz,
L377: method,
L378: JNI_TRUE /* isStatic */);
L379: if (method_object == NULL) {
L380: return JVMTI_ERROR_NONE; /* Bad jmethodID ? This
will be handled elsewhere */
L381: }
Here we are using parameter 'clazz' to find the method_object for
parameter 'method' so that we can validate that 'clazz' refers to
method's class or superclass.
When a bogus 'clazz' value is passed in by a JCK test, the only
reason that JNI ToReflectedMethod() can still find the right
method_object is that our (HotSpot) implementation of JNI
ToReflectedMethod() doesn't really require the 'clazz' parameter
to find the right method_object. So the 'method_object' that we
return is the real one which has a 'clazz' field that doesn't
match the 'clazz' parameter.
Wow does that twist your head around or what?
So we're trusting JNI ToReflectedMethod() to return the right
method_object when we give it a potentially bad 'clazz' value.
So should we use JNI FromReflectedMethod() to convert the
method_object back into a jmethodID and verify that jmethodID
matches the one that we passed to check_methodClass()?
I might be too paranoid here so feel free to say that enough is
enough with this fix.
Thumbs up!
Dan
The fix was tested with the two failing JCK vm/jdwp tests listed
in the bug, the JCK Lang, VM, and API tests, the hotspot JTReg
tests, the java/lang, java/util and other JTReg tests, the
co-located and non-colocated NSK tests, and with the RBT Tier2 tests.
Thanks, Harold
Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
On 9/15/16 2:05 PM, harold seigel wrote:
Hi Dan,
One could argue that a spec compliant JNI implementation wouldn't need
this change in the first place...
That's a completely different discussion. And we don't want to go down
that rat hole again... :-)
Regardless, I'm withdrawing this change because I found that it fails
a com/sun/jdi JTreg test involving static methods in interfaces.
Ouch! OK a test failure for this obscure corner is unexpected... :-(
Dan
Thanks, Harold
On 9/15/2016 3:37 PM, Daniel D. Daugherty wrote:
On 9/15/16 12:10 PM, harold seigel wrote:
(Adding hotspot-runtime)
Hi Dan,
Thanks for looking at this.
I could pass NULL instead of clazz to ToReflectMethod() to ensure
that the method object isn't being obtained from clazz.
I don't think that would be a JNI spec compliant use of the
JNI ToReflectedMethod() function. That would be relying on
the fact that HotSpot doesn't use the clazz parameter to
convert {clazz,jmethodID} => method_object.
Sorry... again...
Dan
Harold
On 9/15/2016 1:09 PM, Daniel D. Daugherty wrote:
On 9/15/16 9:31 AM, harold seigel wrote:
Hi,
Please review this small fix for JDK-8160987. The JDWP
InvokeStatic() method was depending on the JNI function that it
called to enforce the requirement that the specified method must
be a member of the specified class or one of its super classes.
But, JNI does not enforce this requirement. This fix adds code to
JDWP to do its own check that the specified method is a member of
the specified class or one of its super classes.
JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8160987
Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8160987/
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
Sorry I didn't think of this comment during the pre-review...
The only "strange" part of this fix is:
L374: /* Get the method object from the method's jmethodID. */
L375: method_object = JNI_FUNC_PTR(env,ToReflectedMethod)(env,
L376: clazz,
L377: method,
L378: JNI_TRUE /* isStatic */);
L379: if (method_object == NULL) {
L380: return JVMTI_ERROR_NONE; /* Bad jmethodID ? This
will be handled elsewhere */
L381: }
Here we are using parameter 'clazz' to find the method_object for
parameter 'method' so that we can validate that 'clazz' refers to
method's class or superclass.
When a bogus 'clazz' value is passed in by a JCK test, the only
reason that JNI ToReflectedMethod() can still find the right
method_object is that our (HotSpot) implementation of JNI
ToReflectedMethod() doesn't really require the 'clazz' parameter
to find the right method_object. So the 'method_object' that we
return is the real one which has a 'clazz' field that doesn't
match the 'clazz' parameter.
Wow does that twist your head around or what?
So we're trusting JNI ToReflectedMethod() to return the right
method_object when we give it a potentially bad 'clazz' value.
So should we use JNI FromReflectedMethod() to convert the
method_object back into a jmethodID and verify that jmethodID
matches the one that we passed to check_methodClass()?
I might be too paranoid here so feel free to say that enough is
enough with this fix.
Thumbs up!
Dan
The fix was tested with the two failing JCK vm/jdwp tests listed
in the bug, the JCK Lang, VM, and API tests, the hotspot JTReg
tests, the java/lang, java/util and other JTReg tests, the
co-located and non-colocated NSK tests, and with the RBT Tier2 tests.
Thanks, Harold
Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
Hi Dan,
One could argue that a spec compliant JNI implementation wouldn't need
this change in the first place...
Regardless, I'm withdrawing this change because I found that it fails a
com/sun/jdi JTreg test involving static methods in interfaces.
Thanks, Harold
On 9/15/2016 3:37 PM, Daniel D. Daugherty wrote:
On 9/15/16 12:10 PM, harold seigel wrote:
(Adding hotspot-runtime)
Hi Dan,
Thanks for looking at this.
I could pass NULL instead of clazz to ToReflectMethod() to ensure
that the method object isn't being obtained from clazz.
I don't think that would be a JNI spec compliant use of the
JNI ToReflectedMethod() function. That would be relying on
the fact that HotSpot doesn't use the clazz parameter to
convert {clazz,jmethodID} => method_object.
Sorry... again...
Dan
Harold
On 9/15/2016 1:09 PM, Daniel D. Daugherty wrote:
On 9/15/16 9:31 AM, harold seigel wrote:
Hi,
Please review this small fix for JDK-8160987. The JDWP
InvokeStatic() method was depending on the JNI function that it
called to enforce the requirement that the specified method must be
a member of the specified class or one of its super classes. But,
JNI does not enforce this requirement. This fix adds code to JDWP
to do its own check that the specified method is a member of the
specified class or one of its super classes.
JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8160987
Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8160987/
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
Sorry I didn't think of this comment during the pre-review...
The only "strange" part of this fix is:
L374: /* Get the method object from the method's jmethodID. */
L375: method_object = JNI_FUNC_PTR(env,ToReflectedMethod)(env,
L376: clazz,
L377: method,
L378: JNI_TRUE /* isStatic */);
L379: if (method_object == NULL) {
L380: return JVMTI_ERROR_NONE; /* Bad jmethodID ? This
will be handled elsewhere */
L381: }
Here we are using parameter 'clazz' to find the method_object for
parameter 'method' so that we can validate that 'clazz' refers to
method's class or superclass.
When a bogus 'clazz' value is passed in by a JCK test, the only
reason that JNI ToReflectedMethod() can still find the right
method_object is that our (HotSpot) implementation of JNI
ToReflectedMethod() doesn't really require the 'clazz' parameter
to find the right method_object. So the 'method_object' that we
return is the real one which has a 'clazz' field that doesn't
match the 'clazz' parameter.
Wow does that twist your head around or what?
So we're trusting JNI ToReflectedMethod() to return the right
method_object when we give it a potentially bad 'clazz' value.
So should we use JNI FromReflectedMethod() to convert the
method_object back into a jmethodID and verify that jmethodID
matches the one that we passed to check_methodClass()?
I might be too paranoid here so feel free to say that enough is
enough with this fix.
Thumbs up!
Dan
The fix was tested with the two failing JCK vm/jdwp tests listed in
the bug, the JCK Lang, VM, and API tests, the hotspot JTReg tests,
the java/lang, java/util and other JTReg tests, the co-located and
non-colocated NSK tests, and with the RBT Tier2 tests.
Thanks, Harold
Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
On 9/15/16 12:10 PM, harold seigel wrote:
(Adding hotspot-runtime)
Hi Dan,
Thanks for looking at this.
I could pass NULL instead of clazz to ToReflectMethod() to ensure that
the method object isn't being obtained from clazz.
I don't think that would be a JNI spec compliant use of the
JNI ToReflectedMethod() function. That would be relying on
the fact that HotSpot doesn't use the clazz parameter to
convert {clazz,jmethodID} => method_object.
Sorry... again...
Dan
Harold
On 9/15/2016 1:09 PM, Daniel D. Daugherty wrote:
On 9/15/16 9:31 AM, harold seigel wrote:
Hi,
Please review this small fix for JDK-8160987. The JDWP
InvokeStatic() method was depending on the JNI function that it
called to enforce the requirement that the specified method must be
a member of the specified class or one of its super classes. But,
JNI does not enforce this requirement. This fix adds code to JDWP
to do its own check that the specified method is a member of the
specified class or one of its super classes.
JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8160987
Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8160987/
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
Sorry I didn't think of this comment during the pre-review...
The only "strange" part of this fix is:
L374: /* Get the method object from the method's jmethodID. */
L375: method_object = JNI_FUNC_PTR(env,ToReflectedMethod)(env,
L376: clazz,
L377: method,
L378: JNI_TRUE /* isStatic */);
L379: if (method_object == NULL) {
L380: return JVMTI_ERROR_NONE; /* Bad jmethodID ? This
will be handled elsewhere */
L381: }
Here we are using parameter 'clazz' to find the method_object for
parameter 'method' so that we can validate that 'clazz' refers to
method's class or superclass.
When a bogus 'clazz' value is passed in by a JCK test, the only
reason that JNI ToReflectedMethod() can still find the right
method_object is that our (HotSpot) implementation of JNI
ToReflectedMethod() doesn't really require the 'clazz' parameter
to find the right method_object. So the 'method_object' that we
return is the real one which has a 'clazz' field that doesn't
match the 'clazz' parameter.
Wow does that twist your head around or what?
So we're trusting JNI ToReflectedMethod() to return the right
method_object when we give it a potentially bad 'clazz' value.
So should we use JNI FromReflectedMethod() to convert the
method_object back into a jmethodID and verify that jmethodID
matches the one that we passed to check_methodClass()?
I might be too paranoid here so feel free to say that enough is
enough with this fix.
Thumbs up!
Dan
The fix was tested with the two failing JCK vm/jdwp tests listed in
the bug, the JCK Lang, VM, and API tests, the hotspot JTReg tests,
the java/lang, java/util and other JTReg tests, the co-located and
non-colocated NSK tests, and with the RBT Tier2 tests.
Thanks, Harold
Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
(Adding hotspot-runtime)
Hi Dan,
Thanks for looking at this.
I could pass NULL instead of clazz to ToReflectMethod() to ensure that
the method object isn't being obtained from clazz.
Harold
On 9/15/2016 1:09 PM, Daniel D. Daugherty wrote:
On 9/15/16 9:31 AM, harold seigel wrote:
Hi,
Please review this small fix for JDK-8160987. The JDWP
InvokeStatic() method was depending on the JNI function that it
called to enforce the requirement that the specified method must be a
member of the specified class or one of its super classes. But, JNI
does not enforce this requirement. This fix adds code to JDWP to do
its own check that the specified method is a member of the specified
class or one of its super classes.
JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8160987
Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8160987/
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
Sorry I didn't think of this comment during the pre-review...
The only "strange" part of this fix is:
L374: /* Get the method object from the method's jmethodID. */
L375: method_object = JNI_FUNC_PTR(env,ToReflectedMethod)(env,
L376: clazz,
L377: method,
L378: JNI_TRUE /* isStatic */);
L379: if (method_object == NULL) {
L380: return JVMTI_ERROR_NONE; /* Bad jmethodID ? This
will be handled elsewhere */
L381: }
Here we are using parameter 'clazz' to find the method_object for
parameter 'method' so that we can validate that 'clazz' refers to
method's class or superclass.
When a bogus 'clazz' value is passed in by a JCK test, the only
reason that JNI ToReflectedMethod() can still find the right
method_object is that our (HotSpot) implementation of JNI
ToReflectedMethod() doesn't really require the 'clazz' parameter
to find the right method_object. So the 'method_object' that we
return is the real one which has a 'clazz' field that doesn't
match the 'clazz' parameter.
Wow does that twist your head around or what?
So we're trusting JNI ToReflectedMethod() to return the right
method_object when we give it a potentially bad 'clazz' value.
So should we use JNI FromReflectedMethod() to convert the
method_object back into a jmethodID and verify that jmethodID
matches the one that we passed to check_methodClass()?
I might be too paranoid here so feel free to say that enough is
enough with this fix.
Thumbs up!
Dan
The fix was tested with the two failing JCK vm/jdwp tests listed in
the bug, the JCK Lang, VM, and API tests, the hotspot JTReg tests,
the java/lang, java/util and other JTReg tests, the co-located and
non-colocated NSK tests, and with the RBT Tier2 tests.
Thanks, Harold
Re: RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
On 9/15/16 9:31 AM, harold seigel wrote:
Hi,
Please review this small fix for JDK-8160987. The JDWP InvokeStatic()
method was depending on the JNI function that it called to enforce the
requirement that the specified method must be a member of the
specified class or one of its super classes. But, JNI does not enforce
this requirement. This fix adds code to JDWP to do its own check that
the specified method is a member of the specified class or one of its
super classes.
JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8160987
Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8160987/
src/jdk.jdwp.agent/share/native/libjdwp/invoker.c
Sorry I didn't think of this comment during the pre-review...
The only "strange" part of this fix is:
L374: /* Get the method object from the method's jmethodID. */
L375: method_object = JNI_FUNC_PTR(env,ToReflectedMethod)(env,
L376: clazz,
L377: method,
L378: JNI_TRUE /* isStatic */);
L379: if (method_object == NULL) {
L380: return JVMTI_ERROR_NONE; /* Bad jmethodID ? This will
be handled elsewhere */
L381: }
Here we are using parameter 'clazz' to find the method_object for
parameter 'method' so that we can validate that 'clazz' refers to
method's class or superclass.
When a bogus 'clazz' value is passed in by a JCK test, the only
reason that JNI ToReflectedMethod() can still find the right
method_object is that our (HotSpot) implementation of JNI
ToReflectedMethod() doesn't really require the 'clazz' parameter
to find the right method_object. So the 'method_object' that we
return is the real one which has a 'clazz' field that doesn't
match the 'clazz' parameter.
Wow does that twist your head around or what?
So we're trusting JNI ToReflectedMethod() to return the right
method_object when we give it a potentially bad 'clazz' value.
So should we use JNI FromReflectedMethod() to convert the
method_object back into a jmethodID and verify that jmethodID
matches the one that we passed to check_methodClass()?
I might be too paranoid here so feel free to say that enough is
enough with this fix.
Thumbs up!
Dan
The fix was tested with the two failing JCK vm/jdwp tests listed in
the bug, the JCK Lang, VM, and API tests, the hotspot JTReg tests, the
java/lang, java/util and other JTReg tests, the co-located and
non-colocated NSK tests, and with the RBT Tier2 tests.
Thanks, Harold
Re: RFR(S): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
Dmitry, I'd reproduced it. I'll check what is happening. -Dmitry On 2016-09-15 15:34, Dmitry Dmitriev wrote: > Hi Dmitry, > > I don't think that this solves the problem. If some test build > testlibrary before TestJpsJar.java, then testlibrary classes will be > outside the folder with JpsHelper class and thus missed in the jar file. > > I can reproduce this problem with your patch applied: > 1) Run sun/tools/jinfo/BasicJInfoTest.java in clean folder > 2) Then run sun/tools/jps/TestJpsJar.java. TestJpsJar.java fails with > following error: > stderr: [Exception in thread "main" java.lang.NoClassDefFoundError: > jdk/testlibrary/ProcessTools > at JpsBase.main(JpsBase.java:73) > Caused by: java.lang.ClassNotFoundException: jdk.testlibrary.ProcessTools > at > jdk.internal.loader.BuiltinClassLoader.loadClass(java.base@9-internal/BuiltinClassLoader.java:366) > > at > jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(java.base@9-internal/ClassLoaders.java:185) > > at > java.lang.ClassLoader.loadClass(java.base@9-internal/ClassLoader.java:424) > ... 1 more > ] > exitValue = 1 > > java.lang.RuntimeException: Expected to get exit value of [0] > > Thanks, > Dmitry > > On 15.09.2016 15:18, Dmitry Samersoff wrote: >> Everybody, >> >> Please, review the small fix. >> >> http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.01/ >> >> The problem: >> >> TestJpsJar attempts to copy all directories found in test.class.path >> into a single jar file. >> >> It's not necessary and could lead to intermittent ClassNotFound >> exceptions. >> >> Solution: >> >> Jar only a directory with required files. >> >> -Dmitry >> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
RFR(S) 8160987: JDWP ClassType.InvokeMethod doesn't validate class
Hi, Please review this small fix for JDK-8160987. The JDWP InvokeStatic() method was depending on the JNI function that it called to enforce the requirement that the specified method must be a member of the specified class or one of its super classes. But, JNI does not enforce this requirement. This fix adds code to JDWP to do its own check that the specified method is a member of the specified class or one of its super classes. JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8160987 Open webrev: http://cr.openjdk.java.net/~hseigel/bug_8160987/ The fix was tested with the two failing JCK vm/jdwp tests listed in the bug, the JCK Lang, VM, and API tests, the hotspot JTReg tests, the java/lang, java/util and other JTReg tests, the co-located and non-colocated NSK tests, and with the RBT Tier2 tests. Thanks, Harold
Re: RFR(s) 8153711: [REDO] JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command
On 9/15/16 8:18 AM, Severin Gehwolf wrote: On Thu, 2016-09-15 at 08:15 -0600, Daniel D. Daugherty wrote: Dmitry, This fix needs to be run through the entire JPDA test stack before it is pushed. Don't know if we still have test definitions to support that style of run anymore so it might be easier to run it through the equivalent of a JDK9-hs nightly. Hmm, it's been pushed already: http://hg.openjdk.java.net/jdk9/hs/jdk/rev/4c843eb35b8a Sigh... I haven't gotten that far in clearing my email queue this AM. Hopefully we don't see failures like we did with the earlier rounds. Dan Cheers, Severin Dan On 9/14/16 11:50 AM, Dmitry Samersoff wrote: Severin, The fix looks good for me. I'll sponsor the push, but please wait for Serguei. -Dmitry On 2016-09-09 19:27, Severin Gehwolf wrote: Hi, Could I please get a review of the this 4th version of this fix: Bug: https://bugs.openjdk.java.net/browse/JDK-8153711 webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.03/ It fixes a memory leak problem in the debugger as shown by the new regression test. A bit of history to this new patch. The first version[1] of this patch, pushed as fix for JDK-4858370, caused regressions in InterfaceMethodsTest, InvokeTest and OomDebugTest (part of the fix). The cause was not holding the invoker lock when clearing the references. A subsequent version[2] caused deadlocks, because we were holding the invoker lock while invoking in invoker_doInvoke(). Finally, a third version[3] caused NPE's and asserts on Solaris. The reason for them seems to be clearing the request->clazz and request- instance members *after* sending back the reply to the client. My hypothesis is that it maybe related to the sequence of monitor_exit- perform IO->monitor_enter->toss references. Perhaps there is a schedule where the response has been sent back, the next invoke starts for the same app thread and it is just far enough along so that the tossing of the references becomes observable by the next request. Unfortunately, I don't have proof for this. However, testing showed that tossing request->clazz and request- instance members before the IO operation prevents this assertion from being triggered. Thus, I'm now clearing global references - the ones we can clear before sending back the response to the client - in the same block while still holding the invoker and event handler locks as the rest of the operations being done in completeInvokeRequest. Once the response has been sent to the client there are still two global references to clear: The one for the return value and a possible exception which might have occurred. Since they are required for sending the response to the client we do this after it's been sent. I think not holding the invoker lock while invoking is still a problem, but that's being tracked in JDK-8156498. Testing done: - com/sun/jdi test-set. No regressions. New OomDebugTest passes. Fails before. - I haven't observed hangs or regressions in 1000 runs on com/sun/jdi/InvokeTest.java com/sun/jdi/InvokeHangTest.java com/sun/jdi/OomDebugTest.java on Linux x86_64 release/fastdebug - I haven't seen asserts being triggered on Solaris x86_64 fastdebug of 100 iterations of: com/sun/jdi/InvokeTest.java com/sun/jdi/InvokeHangTest.java com/sun/jdi/OomDebugTest.java I believe I need a sponsor who can push this fix through JPRT once reviewed. Thoughts? Thanks, Severin [1] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/277d7584fa03 [2] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.01/ [3] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.02/
Re: RFR(s) 8153711: [REDO] JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command
On Thu, 2016-09-15 at 08:15 -0600, Daniel D. Daugherty wrote: > Dmitry, > > This fix needs to be run through the entire JPDA test stack > before it is pushed. Don't know if we still have test definitions > to support that style of run anymore so it might be easier to > run it through the equivalent of a JDK9-hs nightly. Hmm, it's been pushed already: http://hg.openjdk.java.net/jdk9/hs/jdk/rev/4c843eb35b8a Cheers, Severin > Dan > > On 9/14/16 11:50 AM, Dmitry Samersoff wrote: > > > > Severin, > > > > The fix looks good for me. > > > > I'll sponsor the push, but please wait for Serguei. > > > > -Dmitry > > > > > > On 2016-09-09 19:27, Severin Gehwolf wrote: > > > > > > Hi, > > > > > > Could I please get a review of the this 4th version of this fix: > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8153711 > > > webrev: > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.03/ > > > > > > It fixes a memory leak problem in the debugger as shown by the new > > > regression test. > > > > > > A bit of history to this new patch. The first version[1] of this patch, > > > pushed as fix for JDK-4858370, caused regressions in > > > InterfaceMethodsTest, InvokeTest and OomDebugTest (part of the fix). > > > The cause was not holding the invoker lock when clearing the > > > references. A subsequent version[2] caused deadlocks, because we were > > > holding the invoker lock while invoking in invoker_doInvoke(). > > > > > > Finally, a third version[3] caused NPE's and asserts on Solaris. The > > > reason for them seems to be clearing the request->clazz and request- > > > > > > > > instance members *after* sending back the reply to the client. My > > > hypothesis is that it maybe related to the sequence of monitor_exit- > > > > > > > > perform IO->monitor_enter->toss references. Perhaps there is a > > > schedule where the response has been sent back, the next invoke starts > > > for the same app thread and it is just far enough along so that the > > > tossing of the references becomes observable by the next request. > > > Unfortunately, I don't have proof for this. > > > > > > However, testing showed that tossing request->clazz and request- > > > > > > > > instance members before the IO operation prevents this assertion from > > > being triggered. Thus, I'm now clearing global references - the ones we > > > can clear before sending back the response to the client - in the same > > > block while still holding the invoker and event handler locks as the > > > rest of the operations being done in completeInvokeRequest. Once the > > > response has been sent to the client there are still two global > > > references to clear: The one for the return value and a possible > > > exception which might have occurred. Since they are required for > > > sending the response to the client we do this after it's been sent. > > > > > > I think not holding the invoker lock while invoking is still a problem, > > > but that's being tracked in JDK-8156498. > > > > > > Testing done: > > > > > > - com/sun/jdi test-set. No regressions. > > > New OomDebugTest passes. Fails before. > > > - I haven't observed hangs or regressions in 1000 runs on > > > com/sun/jdi/InvokeTest.java > > > com/sun/jdi/InvokeHangTest.java > > > com/sun/jdi/OomDebugTest.java on Linux x86_64 release/fastdebug > > > - I haven't seen asserts being triggered on Solaris x86_64 > > > fastdebug of 100 iterations of: > > > com/sun/jdi/InvokeTest.java > > > com/sun/jdi/InvokeHangTest.java > > > com/sun/jdi/OomDebugTest.java > > > > > > I believe I need a sponsor who can push this fix through JPRT once > > > reviewed. > > > > > > Thoughts? > > > > > > Thanks, > > > Severin > > > > > > [1] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/277d7584fa03 > > > [2] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.01/ > > > [3] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.02/ > > > > > >
Re: RFR(s) 8153711: [REDO] JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command
Dmitry, This fix needs to be run through the entire JPDA test stack before it is pushed. Don't know if we still have test definitions to support that style of run anymore so it might be easier to run it through the equivalent of a JDK9-hs nightly. Dan On 9/14/16 11:50 AM, Dmitry Samersoff wrote: Severin, The fix looks good for me. I'll sponsor the push, but please wait for Serguei. -Dmitry On 2016-09-09 19:27, Severin Gehwolf wrote: Hi, Could I please get a review of the this 4th version of this fix: Bug: https://bugs.openjdk.java.net/browse/JDK-8153711 webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.03/ It fixes a memory leak problem in the debugger as shown by the new regression test. A bit of history to this new patch. The first version[1] of this patch, pushed as fix for JDK-4858370, caused regressions in InterfaceMethodsTest, InvokeTest and OomDebugTest (part of the fix). The cause was not holding the invoker lock when clearing the references. A subsequent version[2] caused deadlocks, because we were holding the invoker lock while invoking in invoker_doInvoke(). Finally, a third version[3] caused NPE's and asserts on Solaris. The reason for them seems to be clearing the request->clazz and request- instance members *after* sending back the reply to the client. My hypothesis is that it maybe related to the sequence of monitor_exit- perform IO->monitor_enter->toss references. Perhaps there is a schedule where the response has been sent back, the next invoke starts for the same app thread and it is just far enough along so that the tossing of the references becomes observable by the next request. Unfortunately, I don't have proof for this. However, testing showed that tossing request->clazz and request- instance members before the IO operation prevents this assertion from being triggered. Thus, I'm now clearing global references - the ones we can clear before sending back the response to the client - in the same block while still holding the invoker and event handler locks as the rest of the operations being done in completeInvokeRequest. Once the response has been sent to the client there are still two global references to clear: The one for the return value and a possible exception which might have occurred. Since they are required for sending the response to the client we do this after it's been sent. I think not holding the invoker lock while invoking is still a problem, but that's being tracked in JDK-8156498. Testing done: - com/sun/jdi test-set. No regressions. New OomDebugTest passes. Fails before. - I haven't observed hangs or regressions in 1000 runs on com/sun/jdi/InvokeTest.java com/sun/jdi/InvokeHangTest.java com/sun/jdi/OomDebugTest.java on Linux x86_64 release/fastdebug - I haven't seen asserts being triggered on Solaris x86_64 fastdebug of 100 iterations of: com/sun/jdi/InvokeTest.java com/sun/jdi/InvokeHangTest.java com/sun/jdi/OomDebugTest.java I believe I need a sponsor who can push this fix through JPRT once reviewed. Thoughts? Thanks, Severin [1] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/277d7584fa03 [2] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.01/ [3] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.02/
Re: RFR(S): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
Hi Dmitry, I wonder whether failures can be caused by adding the classes in the wrong order... If you have -cp a:b:c - the class loader will look at a first, then b, then c. But if you create a jar and adds a first, b second, and c last, then it's equivalent to -cp c:b:a Could there be an issue there? best regards, -- daniel On 15/09/16 13:18, Dmitry Samersoff wrote: Everybody, Please, review the small fix. http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.01/ The problem: TestJpsJar attempts to copy all directories found in test.class.path into a single jar file. It's not necessary and could lead to intermittent ClassNotFound exceptions. Solution: Jar only a directory with required files. -Dmitry
Re: RFR(S): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
Hi Dmitry, I don't think that this solves the problem. If some test build testlibrary before TestJpsJar.java, then testlibrary classes will be outside the folder with JpsHelper class and thus missed in the jar file. I can reproduce this problem with your patch applied: 1) Run sun/tools/jinfo/BasicJInfoTest.java in clean folder 2) Then run sun/tools/jps/TestJpsJar.java. TestJpsJar.java fails with following error: stderr: [Exception in thread "main" java.lang.NoClassDefFoundError: jdk/testlibrary/ProcessTools at JpsBase.main(JpsBase.java:73) Caused by: java.lang.ClassNotFoundException: jdk.testlibrary.ProcessTools at jdk.internal.loader.BuiltinClassLoader.loadClass(java.base@9-internal/BuiltinClassLoader.java:366) at jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(java.base@9-internal/ClassLoaders.java:185) at java.lang.ClassLoader.loadClass(java.base@9-internal/ClassLoader.java:424) ... 1 more ] exitValue = 1 java.lang.RuntimeException: Expected to get exit value of [0] Thanks, Dmitry On 15.09.2016 15:18, Dmitry Samersoff wrote: Everybody, Please, review the small fix. http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.01/ The problem: TestJpsJar attempts to copy all directories found in test.class.path into a single jar file. It's not necessary and could lead to intermittent ClassNotFound exceptions. Solution: Jar only a directory with required files. -Dmitry
RFR(S): JDK-8165500: TestJpsJar shouldn't jar all test.classpath directories
Everybody, Please, review the small fix. http://cr.openjdk.java.net/~dsamersoff/JDK-8165500/webrev.01/ The problem: TestJpsJar attempts to copy all directories found in test.class.path into a single jar file. It's not necessary and could lead to intermittent ClassNotFound exceptions. Solution: Jar only a directory with required files. -Dmitry -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR: JDK-8068155: [Findbugs]new sun.jvm.hotspot.utilities.ObjectReader() creates a sun.jvm.hotspot.utilities.ProcImageClassLoader classloader, which should be performed within a doPrivileged block
+1
PS.
* SA module does not export any package - as of now, only jhsdb is the
only way to use SA. So, most findbugs findings are noise. i.e., user
code or any code outside SA module cannot access SA programmatically.
Only way is to use command line switches of Java launcher - if untrusted
code can do that [it cannot!], all doomed anyway :)
-Sundar
On 9/15/2016 4:19 PM, Dmitry Samersoff wrote:
> Sharath,
>
> I don't see any requirements that ObjectReader should run with an
> application that install security manager but doesn't have
> RuntimePermission.createClassLoader
>
> So I would recommend to close this bug as "not an issue".
>
> -Dmitry
>
> On 2016-09-15 06:38, Harsha wardhana B wrote:
>> Hello,
>>
>> It is not required that SA should be run under security manager to
>> address this change. Any standalone application when run under security
>> manager can use ObjectReader class to exploit vulnerabilities. That is
>> something that should be evaluated.
>>
>> With the below fix any application when run under security manager
>> without RuntimePermission.createClassLoader will be able to create
>> ProcImageClassLoader. We need to check if it is something that is
>> desired and what vulnerabilities can be exploited, if any.
>>
>> -Harsha
>>
>> On 9/14/2016 5:58 PM, Sharath Ballal wrote:
>>> David,
That aside, the code uses raw types, which is bad. It should also be
able to retain the this(...) invocation e.g (I haven't compiled this):
>>> This works, Thanks.
>>>
>>>
>>> -Sharath Ballal
>>>
>>>
>>>
>>> -Original Message-
>>> From: David Holmes
>>> Sent: Wednesday, September 14, 2016 3:07 PM
>>> To: Sharath Ballal;serviceability-dev@openjdk.java.net
>>> Subject: Re: RFR: JDK-8068155: [Findbugs]new
>>> sun.jvm.hotspot.utilities.ObjectReader() creates a
>>> sun.jvm.hotspot.utilities.ProcImageClassLoader classloader, which
>>> should be performed within a doPrivileged block
>>>
>>> Hi Sharath,
>>>
>>> On 14/09/2016 6:14 PM, Sharath Ballal wrote:
Hello,
Please review this fix to add creation of classloader code into
doPrivileged block
Issue:https://bugs.openjdk.java.net/browse/JDK-8068155
Webrev:http://cr.openjdk.java.net/~sballal/8068155/webrev.00/
>>> First I'm also curious about why FindBugs thinks this is needed. AFAIK
>>> you use the doPrivileged to allow you to create the classLoader when
>>> it would otherwise fail if a SecurityManager were present.
>>>
>>> That aside, the code uses raw types, which is bad. It should also be
>>> able to retain the this(...) invocation e.g (I haven't compiled this):
>>>
>>> public ObjectReader() {
>>> this(AccessController.doPrivileged(
>>>new PrivilegedAction() {
>>> public ClassLoader run() {
>>> return new ProcImageClassLoader();
>>> }
>>>}
>>> ));
>>> }
>>>
>>> Thanks,
>>> David
>>>
-Sharath Ballal
>
Re: RFR: JDK-8068155: [Findbugs]new sun.jvm.hotspot.utilities.ObjectReader() creates a sun.jvm.hotspot.utilities.ProcImageClassLoader classloader, which should be performed within a doPrivileged block
Sharath,
I don't see any requirements that ObjectReader should run with an
application that install security manager but doesn't have
RuntimePermission.createClassLoader
So I would recommend to close this bug as "not an issue".
-Dmitry
On 2016-09-15 06:38, Harsha wardhana B wrote:
> Hello,
>
> It is not required that SA should be run under security manager to
> address this change. Any standalone application when run under security
> manager can use ObjectReader class to exploit vulnerabilities. That is
> something that should be evaluated.
>
> With the below fix any application when run under security manager
> without RuntimePermission.createClassLoader will be able to create
> ProcImageClassLoader. We need to check if it is something that is
> desired and what vulnerabilities can be exploited, if any.
>
> -Harsha
>
> On 9/14/2016 5:58 PM, Sharath Ballal wrote:
>> David,
>>> That aside, the code uses raw types, which is bad. It should also be
>>> able to retain the this(...) invocation e.g (I haven't compiled this):
>> This works, Thanks.
>>
>>
>> -Sharath Ballal
>>
>>
>>
>> -Original Message-
>> From: David Holmes
>> Sent: Wednesday, September 14, 2016 3:07 PM
>> To: Sharath Ballal;serviceability-dev@openjdk.java.net
>> Subject: Re: RFR: JDK-8068155: [Findbugs]new
>> sun.jvm.hotspot.utilities.ObjectReader() creates a
>> sun.jvm.hotspot.utilities.ProcImageClassLoader classloader, which
>> should be performed within a doPrivileged block
>>
>> Hi Sharath,
>>
>> On 14/09/2016 6:14 PM, Sharath Ballal wrote:
>>> Hello,
>>>
>>> Please review this fix to add creation of classloader code into
>>> doPrivileged block
>>>
>>> Issue:https://bugs.openjdk.java.net/browse/JDK-8068155
>>>
>>> Webrev:http://cr.openjdk.java.net/~sballal/8068155/webrev.00/
>> First I'm also curious about why FindBugs thinks this is needed. AFAIK
>> you use the doPrivileged to allow you to create the classLoader when
>> it would otherwise fail if a SecurityManager were present.
>>
>> That aside, the code uses raw types, which is bad. It should also be
>> able to retain the this(...) invocation e.g (I haven't compiled this):
>>
>> public ObjectReader() {
>> this(AccessController.doPrivileged(
>>new PrivilegedAction() {
>> public ClassLoader run() {
>> return new ProcImageClassLoader();
>> }
>>}
>> ));
>> }
>>
>> Thanks,
>> David
>>
>>> -Sharath Ballal
>>>
>>>
>>>
>>>
>>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Re: RFR: JDK-8068155: [Findbugs]new sun.jvm.hotspot.utilities.ObjectReader() creates a sun.jvm.hotspot.utilities.ProcImageClassLoader classloader, which should be performed within a doPrivileged block
Hello,
It is not required that SA should be run under security manager to
address this change. Any standalone application when run under security
manager can use ObjectReader class to exploit vulnerabilities. That is
something that should be evaluated.
With the below fix any application when run under security manager
without RuntimePermission.createClassLoader will be able to create
ProcImageClassLoader. We need to check if it is something that is
desired and what vulnerabilities can be exploited, if any.
-Harsha
On 9/14/2016 5:58 PM, Sharath Ballal wrote:
David,
That aside, the code uses raw types, which is bad. It should also be able to
retain the this(...) invocation e.g (I haven't compiled this):
This works, Thanks.
-Sharath Ballal
-Original Message-
From: David Holmes
Sent: Wednesday, September 14, 2016 3:07 PM
To: Sharath Ballal;serviceability-dev@openjdk.java.net
Subject: Re: RFR: JDK-8068155: [Findbugs]new
sun.jvm.hotspot.utilities.ObjectReader() creates a
sun.jvm.hotspot.utilities.ProcImageClassLoader classloader, which should be
performed within a doPrivileged block
Hi Sharath,
On 14/09/2016 6:14 PM, Sharath Ballal wrote:
Hello,
Please review this fix to add creation of classloader code into
doPrivileged block
Issue:https://bugs.openjdk.java.net/browse/JDK-8068155
Webrev:http://cr.openjdk.java.net/~sballal/8068155/webrev.00/
First I'm also curious about why FindBugs thinks this is needed. AFAIK you use
the doPrivileged to allow you to create the classLoader when it would otherwise
fail if a SecurityManager were present.
That aside, the code uses raw types, which is bad. It should also be able to
retain the this(...) invocation e.g (I haven't compiled this):
public ObjectReader() {
this(AccessController.doPrivileged(
new PrivilegedAction() {
public ClassLoader run() {
return new ProcImageClassLoader();
}
}
));
}
Thanks,
David
-Sharath Ballal
Re: [PING] RFR(s) 8153711: [REDO] JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command
Hi Serguei, On Wed, 2016-09-14 at 11:44 -0700, serguei.spit...@oracle.com wrote: > Hi Severin, > > The fix looks good. > Thank you for persistence in fixing the issue! Thanks for the review! > The only suggestion is to refactor the lines 800-815 into a method call. > Something like deletePoentiallySavedGlobalRefs, similar to > deleteGlobalArgumetRefs. Sure. Updated webrev is here: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.04/ If that looks good, here would be the HG exported version: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/JDK-8153711-jdk9-rc-jdk.export.patch Cheers, Severin > Thanks, > Serguei > > > > On 9/14/16 09:34, Severin Gehwolf wrote: > > > > Anyone? > > > > On Fri, 2016-09-09 at 18:27 +0200, Severin Gehwolf wrote: > > > > > > Hi, > > > > > > Could I please get a review of the this 4th version of this fix: > > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8153711 > > > webrev: > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.03/ > > > > > > It fixes a memory leak problem in the debugger as shown by the new > > > regression test. > > > > > > A bit of history to this new patch. The first version[1] of this patch, > > > pushed as fix for JDK-4858370, caused regressions in > > > InterfaceMethodsTest, InvokeTest and OomDebugTest (part of the fix). > > > The cause was not holding the invoker lock when clearing the > > > references. A subsequent version[2] caused deadlocks, because we were > > > holding the invoker lock while invoking in invoker_doInvoke(). > > > > > > Finally, a third version[3] caused NPE's and asserts on Solaris. The > > > reason for them seems to be clearing the request->clazz and request- > > > > > > > > instance members *after* sending back the reply to the client. My > > > hypothesis is that it maybe related to the sequence of monitor_exit- > > > > > > > > perform IO->monitor_enter->toss references. Perhaps there is a > > > schedule where the response has been sent back, the next invoke starts > > > for the same app thread and it is just far enough along so that the > > > tossing of the references becomes observable by the next request. > > > Unfortunately, I don't have proof for this. > > > > > > However, testing showed that tossing request->clazz and request- > > > > > > > > instance members before the IO operation prevents this assertion from > > > being triggered. Thus, I'm now clearing global references - the ones we > > > can clear before sending back the response to the client - in the same > > > block while still holding the invoker and event handler locks as the > > > rest of the operations being done in completeInvokeRequest. Once the > > > response has been sent to the client there are still two global > > > references to clear: The one for the return value and a possible > > > exception which might have occurred. Since they are required for > > > sending the response to the client we do this after it's been sent. > > > > > > I think not holding the invoker lock while invoking is still a problem, > > > but that's being tracked in JDK-8156498. > > > > > > Testing done: > > > > > > - com/sun/jdi test-set. No regressions. > > > New OomDebugTest passes. Fails before. > > > - I haven't observed hangs or regressions in 1000 runs on > > > com/sun/jdi/InvokeTest.java > > > com/sun/jdi/InvokeHangTest.java > > > com/sun/jdi/OomDebugTest.java on Linux x86_64 release/fastdebug > > > - I haven't seen asserts being triggered on Solaris x86_64 > > > fastdebug of 100 iterations of: > > > com/sun/jdi/InvokeTest.java > > > com/sun/jdi/InvokeHangTest.java > > > com/sun/jdi/OomDebugTest.java > > > > > > I believe I need a sponsor who can push this fix through JPRT once > > > reviewed. > > > > > > Thoughts? > > > > > > Thanks, > > > Severin > > > > > > [1] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/277d7584fa03 > > > [2] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.01/ > > > [3] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev.02/ >
Re: RFR(s) 8153711: [REDO] JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command
On Wed, 2016-09-14 at 20:50 +0300, Dmitry Samersoff wrote: > Severin, > > The fix looks good for me. > > I'll sponsor the push, but please wait for Serguei. Thanks, Dmitry. Cheers, Severin > -Dmitry > > > On 2016-09-09 19:27, Severin Gehwolf wrote: > > > > Hi, > > > > Could I please get a review of the this 4th version of this fix: > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8153711 > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/we > > brev.03/ > > > > It fixes a memory leak problem in the debugger as shown by the new > > regression test. > > > > A bit of history to this new patch. The first version[1] of this > > patch, > > pushed as fix for JDK-4858370, caused regressions in > > InterfaceMethodsTest, InvokeTest and OomDebugTest (part of the > > fix). > > The cause was not holding the invoker lock when clearing the > > references. A subsequent version[2] caused deadlocks, because we > > were > > holding the invoker lock while invoking in invoker_doInvoke(). > > > > Finally, a third version[3] caused NPE's and asserts on Solaris. > > The > > reason for them seems to be clearing the request->clazz and > > request- > > > > > > instance members *after* sending back the reply to the client. My > > hypothesis is that it maybe related to the sequence of > > monitor_exit- > > > > > > perform IO->monitor_enter->toss references. Perhaps there is a > > schedule where the response has been sent back, the next invoke > > starts > > for the same app thread and it is just far enough along so that the > > tossing of the references becomes observable by the next request. > > Unfortunately, I don't have proof for this. > > > > However, testing showed that tossing request->clazz and request- > > > > > > instance members before the IO operation prevents this assertion > > > from > > being triggered. Thus, I'm now clearing global references - the > > ones we > > can clear before sending back the response to the client - in the > > same > > block while still holding the invoker and event handler locks as > > the > > rest of the operations being done in completeInvokeRequest. Once > > the > > response has been sent to the client there are still two global > > references to clear: The one for the return value and a possible > > exception which might have occurred. Since they are required for > > sending the response to the client we do this after it's been sent. > > > > I think not holding the invoker lock while invoking is still a > > problem, > > but that's being tracked in JDK-8156498. > > > > Testing done: > > > > - com/sun/jdi test-set. No regressions. > > New OomDebugTest passes. Fails before. > > - I haven't observed hangs or regressions in 1000 runs on > > com/sun/jdi/InvokeTest.java > > com/sun/jdi/InvokeHangTest.java > > com/sun/jdi/OomDebugTest.java on Linux x86_64 release/fastdebug > > - I haven't seen asserts being triggered on Solaris x86_64 > > fastdebug of 100 iterations of: > > com/sun/jdi/InvokeTest.java > > com/sun/jdi/InvokeHangTest.java > > com/sun/jdi/OomDebugTest.java > > > > I believe I need a sponsor who can push this fix through JPRT once > > reviewed. > > > > Thoughts? > > > > Thanks, > > Severin > > > > [1] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/277d7584fa03 > > [2] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev > > .01/ > > [3] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8153711/webrev > > .02/ > > > >
