Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM
On Mon, 28 Mar 2022 06:19:39 GMT, David Holmes wrote:
>> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
>> arguments.cpp, which aborts the VM when it fails to allocate a string copy
>> of the property value.
>>
>>
>> bool PathString::set_value(const char *value) {
>> if (_value != NULL) {
>> FreeHeap(_value);
>> }
>> _value = AllocateHeap(strlen(value)+1, mtArguments );
>> // should pass AllocFailStrategy::RETURN_NULL -^
>> assert(_value != NULL, "Unable to allocate space for new path value");
>>
>>
>> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
>> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
>> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
>
> Hi Ioi,
>
> I think the real bug in this code is that
>
> `_value = AllocateHeap(strlen(value)+1, mtArguments);`
>
> should be:
>
> `_value = AllocateHeap(strlen(value)+1, mtArguments,
> AllocFailStrategy::RETURN_NULL);`
>
> as this code is used from `JvmtiEnv::SetSystemProperty` and it seems wrong to
> abort the VM on that path.
Thanks @dholmes-ora and @sspitsyn for the review.
-
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM [v4]
On Tue, 29 Mar 2022 15:41:29 GMT, Ioi Lam wrote:
>> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
>> arguments.cpp, which aborts the VM when it fails to allocate a string copy
>> of the property value.
>>
>>
>> bool PathString::set_value(const char *value) {
>> if (_value != NULL) {
>> FreeHeap(_value);
>> }
>> _value = AllocateHeap(strlen(value)+1, mtArguments );
>> // should pass AllocFailStrategy::RETURN_NULL -^
>> assert(_value != NULL, "Unable to allocate space for new path value");
>>
>>
>> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
>> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
>> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
>
> Ioi Lam has updated the pull request with a new target base due to a merge or
> a rebase. The incremental webrev excludes the unrelated changes brought in by
> the merge/rebase. The pull request contains four additional commits since the
> last revision:
>
> - Merge branch 'master' of https://github.com/openjdk/jdk into
> 8207025-simplify-pathstring-setvalue
> - @dholmes-ora comments: simplify the changes
> - @dholmes-ora comments: changed implementation to work with
> JvmtiEnv::SetSystemProperty
> - 8207025: Simplify PathString::set_value() in arguments.cpp
Marked as reviewed by amenkov (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM [v4]
> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
> arguments.cpp, which aborts the VM when it fails to allocate a string copy of
> the property value.
>
>
> bool PathString::set_value(const char *value) {
> if (_value != NULL) {
> FreeHeap(_value);
> }
> _value = AllocateHeap(strlen(value)+1, mtArguments );
> // should pass AllocFailStrategy::RETURN_NULL -^
> assert(_value != NULL, "Unable to allocate space for new path value");
>
>
> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
Ioi Lam has updated the pull request with a new target base due to a merge or a
rebase. The incremental webrev excludes the unrelated changes brought in by the
merge/rebase. The pull request contains four additional commits since the last
revision:
- Merge branch 'master' of https://github.com/openjdk/jdk into
8207025-simplify-pathstring-setvalue
- @dholmes-ora comments: simplify the changes
- @dholmes-ora comments: changed implementation to work with
JvmtiEnv::SetSystemProperty
- 8207025: Simplify PathString::set_value() in arguments.cpp
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7981/files
- new: https://git.openjdk.java.net/jdk/pull/7981/files/14a44f63..9fa4cc50
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7981=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7981=02-03
Stats: 1781 lines in 82 files changed: 1348 ins; 78 del; 355 mod
Patch: https://git.openjdk.java.net/jdk/pull/7981.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7981/head:pull/7981
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM [v3]
On Tue, 29 Mar 2022 05:24:59 GMT, Ioi Lam wrote:
>> src/hotspot/share/runtime/arguments.hpp line 113:
>>
>>> 111: bool writeable() const { return _writeable; }
>>> 112:
>>> 113: bool readable() const {
>>
>> Might be better/simpler to keep is_readable and change to is_writeable(), as
>> that avoids changing other files.
>
> There's also `internal()`, which would need to be changed to `is_internal()`
> as well for consistency. I chose to change `is_readable()` to `readable()` so
> I just needed to change one name.
Okay.
-
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM [v3]
On Tue, 29 Mar 2022 04:24:15 GMT, Ioi Lam wrote:
>> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
>> arguments.cpp, which aborts the VM when it fails to allocate a string copy
>> of the property value.
>>
>>
>> bool PathString::set_value(const char *value) {
>> if (_value != NULL) {
>> FreeHeap(_value);
>> }
>> _value = AllocateHeap(strlen(value)+1, mtArguments );
>> // should pass AllocFailStrategy::RETURN_NULL -^
>> assert(_value != NULL, "Unable to allocate space for new path value");
>>
>>
>> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
>> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
>> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> @dholmes-ora comments: simplify the changes
On the JVMTI side this looks good and corrects an oversight.
The SystemProperty API changes are not quite what I expected but I see why you
did them.
Thanks,
David
-
Marked as reviewed by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM [v3]
On Tue, 29 Mar 2022 04:24:15 GMT, Ioi Lam wrote:
>> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
>> arguments.cpp, which aborts the VM when it fails to allocate a string copy
>> of the property value.
>>
>>
>> bool PathString::set_value(const char *value) {
>> if (_value != NULL) {
>> FreeHeap(_value);
>> }
>> _value = AllocateHeap(strlen(value)+1, mtArguments );
>> // should pass AllocFailStrategy::RETURN_NULL -^
>> assert(_value != NULL, "Unable to allocate space for new path value");
>>
>>
>> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
>> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
>> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> @dholmes-ora comments: simplify the changes
Hi Ioi,
The fix looks good to me. I leave the decision about is_readable/readable to
you and David. I have a very little preference to is_* variant for consistency
with other places in HopSpot I'm aware off.
Thank you a lot for taking care about this!
-Serguei
-
Marked as reviewed by sspitsyn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM [v3]
On Tue, 29 Mar 2022 04:58:32 GMT, David Holmes wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @dholmes-ora comments: simplify the changes
>
> src/hotspot/share/runtime/arguments.hpp line 113:
>
>> 111: bool writeable() const { return _writeable; }
>> 112:
>> 113: bool readable() const {
>
> Might be better/simpler to keep is_readable and change to is_writeable(), as
> that avoids changing other files.
There's also `internal()`, which would need to be changed to `is_internal()` as
well for consistency. I chose to change `is_readable()` to `readable()` so I
just needed to change one name.
-
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM [v3]
On Tue, 29 Mar 2022 04:24:15 GMT, Ioi Lam wrote:
>> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
>> arguments.cpp, which aborts the VM when it fails to allocate a string copy
>> of the property value.
>>
>>
>> bool PathString::set_value(const char *value) {
>> if (_value != NULL) {
>> FreeHeap(_value);
>> }
>> _value = AllocateHeap(strlen(value)+1, mtArguments );
>> // should pass AllocFailStrategy::RETURN_NULL -^
>> assert(_value != NULL, "Unable to allocate space for new path value");
>>
>>
>> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
>> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
>> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> @dholmes-ora comments: simplify the changes
src/hotspot/share/runtime/arguments.hpp line 113:
> 111: bool writeable() const { return _writeable; }
> 112:
> 113: bool readable() const {
Might be better/simpler to keep is_readable and change to is_writeable(), as
that avoids changing other files.
-
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM [v3]
> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
> arguments.cpp, which aborts the VM when it fails to allocate a string copy of
> the property value.
>
>
> bool PathString::set_value(const char *value) {
> if (_value != NULL) {
> FreeHeap(_value);
> }
> _value = AllocateHeap(strlen(value)+1, mtArguments );
> // should pass AllocFailStrategy::RETURN_NULL -^
> assert(_value != NULL, "Unable to allocate space for new path value");
>
>
> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
Ioi Lam has updated the pull request incrementally with one additional commit
since the last revision:
@dholmes-ora comments: simplify the changes
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7981/files
- new: https://git.openjdk.java.net/jdk/pull/7981/files/0397499f..14a44f63
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7981=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7981=01-02
Stats: 29 lines in 4 files changed: 1 ins; 14 del; 14 mod
Patch: https://git.openjdk.java.net/jdk/pull/7981.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7981/head:pull/7981
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM [v2]
On Mon, 28 Mar 2022 22:06:12 GMT, David Holmes wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @dholmes-ora comments: changed implementation to work with
>> JvmtiEnv::SetSystemProperty
>
> src/hotspot/share/prims/jvmtiEnv.cpp line 3431:
>
>> 3429: jvmtiError
>> 3430: JvmtiEnv::SetSystemProperty(const char* property, const char*
>> value_ptr) {
>> 3431: NULL_CHECK(property, JVMTI_ERROR_NULL_POINTER);
>
> You don't need this. The null check happens in the wrapper code generated
> from the xml file. See
> build//hotspot/variant-server/gensrc/jvmtifiles/jvmtiEnterTrace.cpp
I agree with David here that this change is not needed.
This is a relevant fragment from the jvmtiEnter.cpp:
static jvmtiError JNICALL
jvmti_SetSystemProperty(jvmtiEnv* env,
const char* property,
const char* value_ptr) {
. . .
if (Threads::number_of_threads() != 0) {
. . .
if (property == NULL) {
return JVMTI_ERROR_NULL_POINTER;
}
err = jvmti_env->SetSystemProperty(property, value_ptr);
} else {
if (property == NULL) {
return JVMTI_ERROR_NULL_POINTER;
}
err = jvmti_env->SetSystemProperty(property, value_ptr);
}
return err;
-
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM [v2]
On Mon, 28 Mar 2022 18:34:31 GMT, Ioi Lam wrote:
>> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
>> arguments.cpp, which aborts the VM when it fails to allocate a string copy
>> of the property value.
>>
>>
>> bool PathString::set_value(const char *value) {
>> if (_value != NULL) {
>> FreeHeap(_value);
>> }
>> _value = AllocateHeap(strlen(value)+1, mtArguments );
>> // should pass AllocFailStrategy::RETURN_NULL -^
>> assert(_value != NULL, "Unable to allocate space for new path value");
>>
>>
>> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
>> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
>> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> @dholmes-ora comments: changed implementation to work with
> JvmtiEnv::SetSystemProperty
Hi Ioi,
I don't think this needs to be that complicated. In set_system_property you can
query if the flag is writeable() directly, so then you don't need a ternary
return value from set_property_value().
Also see comment below.
David
src/hotspot/share/prims/jvmtiEnv.cpp line 3431:
> 3429: jvmtiError
> 3430: JvmtiEnv::SetSystemProperty(const char* property, const char*
> value_ptr) {
> 3431: NULL_CHECK(property, JVMTI_ERROR_NULL_POINTER);
You don't need this. The null check happens in the wrapper code generated from
the xml file. See
build//hotspot/variant-server/gensrc/jvmtifiles/jvmtiEnterTrace.cpp
-
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7981
Re: RFR: 8207025: JvmtiEnv::SetSystemProperty() does not handle OOM
On Mon, 28 Mar 2022 06:19:39 GMT, David Holmes wrote:
>> `JvmtiEnv::SetSystemProperty` eventually calls `PathString::set_value` in
>> arguments.cpp, which aborts the VM when it fails to allocate a string copy
>> of the property value.
>>
>>
>> bool PathString::set_value(const char *value) {
>> if (_value != NULL) {
>> FreeHeap(_value);
>> }
>> _value = AllocateHeap(strlen(value)+1, mtArguments );
>> // should pass AllocFailStrategy::RETURN_NULL -^
>> assert(_value != NULL, "Unable to allocate space for new path value");
>>
>>
>> This should be fixed so that `JvmtiEnv::SetSystemProperty` can return
>> `JVMTI_ERROR_OUT_OF_MEMORY` in case of OOM. See
>> https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
>
> Hi Ioi,
>
> I think the real bug in this code is that
>
> `_value = AllocateHeap(strlen(value)+1, mtArguments);`
>
> should be:
>
> `_value = AllocateHeap(strlen(value)+1, mtArguments,
> AllocFailStrategy::RETURN_NULL);`
>
> as this code is used from `JvmtiEnv::SetSystemProperty` and it seems wrong to
> abort the VM on that path.
@dholmes-ora To get `JvmtiEnv::SetSystemProperty` working properly, the fix is
more complicated. According to the specification
https://docs.oracle.com/en/java/javase/17/docs/specs/jvmti.html#SetSystemProperty
`JvmtiEnv::SetSystemProperty` should return `JVMTI_ERROR_NOT_AVAILABLE` when
the property is not available or is not writeable, and
JVMTI_ERROR_OUT_OF_MEMORY when OOM (this is one of the "universal errors").
Therefore, I had to change `SystemProperty::set_writeable_value` to return a
tri-state result.
I also added the `JVMTI_ERROR_NULL_POINTER` that was in the spec but wasn't
implemented.
@sspitsyn could you take a look as well?
-
PR: https://git.openjdk.java.net/jdk/pull/7981
