RFR: JDK-8147388: Add diagnostic commands to attach JVMTI agent.
Yasumasa Suenaga
yasuenag at gmail.com
Wed Feb 17 11:42:04 UTC 2016
Hi all,
Jaroslav found that a patch for JDK-8147388 cannot build on Windows.
So I fix it in new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8147388/webrev.05/
jtreg test for this feature is passed on Windows i586 and Fedora23 x86_64.
Could you review it again?
Thanks,
Yasumasa
On 2016/01/24 2:37, Dmitry Samersoff wrote:
> Yasumasa,
>
> Looks good for me!
>
> -Dmitry
>
> On 2016-01-23 17:18, Yasumasa Suenaga wrote:
>> Dmitry,
>>
>> Thank you for your explanation.
>> I've added length check with 4096 bytes in new webrev:
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8147388/webrev.04/
>>
>> PATH_MAX is not defined Visual C++ 2015.
>> So I do not use this macro.
>>
>>
>> Could you review again?
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2016/01/22 18:41, Dmitry Samersoff wrote:
>>> Yasumasa,
>>>
>>> It's dangerous to allow arbitrary length string to be passed to running
>>> process. We should limit it to some reasonable value.
>>>
>>> It is much easier to increase the limit if necessary than debug random
>>> allocation failures caused by too long string.
>>>
>>> e.g.
>>>
>>> Corrupted script (or admin mistake) send a 1Gb of garbage to VM as an
>>> options. VM successfully allocate memory for options within DCMD but OOM
>>> later somewhere in C2 or JVMTI.
>>>
>>> How long it takes to find real cause of the problem?
>>>
>>> -Dmitry
>>>
>>> On 2016-01-22 11:56, Yasumasa Suenaga wrote:
>>>> Dmitry,
>>>>
>>>> Can we limit the length of argument?
>>>> I guess that we can pass more length when we use -agentlib,
>>>> -javaagent, etc.
>>>> (I do not know about commandline length.)
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>> 2016/01/22 17:45 "Dmitry Samersoff" <dmitry.samersoff at oracle.com
>>>> <mailto:dmitry.samersoff at oracle.com>>:
>>>>
>>>> Yasumasa,
>>>>
>>>> I would prefer to check that opt_len is less than PATH_MAX
>>>> *before* any
>>>> attempt to allocate memory to avoid any possible attack/missuses.
>>>>
>>>> I.e.:
>>>>
>>>> int opt_len = strlen(_libpath.value()) + strlen(_option.value())
>>>> + 2;
>>>> if (opt_len > PATH_MAX) {
>>>> output()->print_cr("JVMTI agent attach failed: "
>>>> "Options is too long.");
>>>> return;
>>>> }
>>>>
>>>> char *opt = (char *)os::malloc(opt_len, mtInternal);
>>>> if (opt == NULL) {
>>>> output()->print_cr("JVMTI agent attach failed: "
>>>> "Could not allocate %d bytes for argument.",
>>>> opt_len);
>>>> }
>>>>
>>>>
>>>> -Dmitry
>>>>
>>>>
>>>> On 2016-01-22 06:21, Yasumasa Suenaga wrote:
>>>> > Hi,
>>>> >
>>>> > I think that we can check malloc error as below:
>>>> > --------------
>>>> > int opt_len = strlen(_libpath.value()) +
>>>> strlen(_option.value()) + 2;
>>>> > char *opt = (char *)os::malloc(opt_len, mtInternal);
>>>> > if (opt == NULL) {
>>>> > // 4096 comes from PATH_MAX at Linux.
>>>> > if (opt_len > 4096) {
>>>> > output()->print_cr("JVMTI agent attach failed: "
>>>> > "Could not allocate %d bytes for
>>>> argument.",
>>>> > opt_len);
>>>> > } else {
>>>> > // VM might not work because memory exhausted.
>>>> > vm_exit_out_of_memory(opt_len, OOM_MALLOC_ERROR,
>>>> > "JVMTIAgentLoadDCmd::execute");
>>>> > }
>>>> > }
>>>> > --------------
>>>> >
>>>> > If you think that this code should NOT be available in dcmd, I
>>>> will remove
>>>> > vm_exit_out_of_memory() from it.
>>>> >
>>>> >
>>>> > Thanks,
>>>> >
>>>> > Yasumasa
>>>> >
>>>> >
>>>> > 2016-01-20 18:06 GMT+09:00 Yasumasa Suenaga <yasuenag at gmail.com
>>>> <mailto:yasuenag at gmail.com>
>>>> > <mailto:yasuenag at gmail.com <mailto:yasuenag at gmail.com>>>:
>>>> >
>>>> > I agree to Dmitry.
>>>> >
>>>> > Most case of malloc error, native memory is exhausted.
>>>> > Thus I think process (or system) is illegal state. It should
>>>> be shut
>>>> > down.
>>>> > If do not so, malloc failure might be occurred another point.
>>>> >
>>>> > However, I think that it is very difficult to set the
>>>> threshold.
>>>> > If it can be clear, I will make a new webrev.
>>>> >
>>>> > Thanks,
>>>> >
>>>> > Yasumasa
>>>> >
>>>> > 2016/01/20 17:03 "Dmitry Samersoff"
>>>> <dmitry.samersoff at oracle.com <mailto:dmitry.samersoff at oracle.com>
>>>> > <mailto:dmitry.samersoff at oracle.com
>>>> <mailto:dmitry.samersoff at oracle.com>>>:
>>>> >
>>>> > David,
>>>> >
>>>> > PS:
>>>> > It might be a good to check opt_len for some large
>>>> enough
>>>> value
>>>> > (like
>>>> > 2048) before allocation attempt and send back a message.
>>>> >
>>>> > -Dmitry
>>>> >
>>>> > On 2016-01-20 08:03, David Holmes wrote:
>>>> > > On 20/01/2016 9:13 AM, Yasumasa Suenaga wrote:
>>>> > >> Hi David,
>>>> > >>
>>>> > >> ShouldNotReachHere( ) is called at NMTDcmd:
>>>> > >>
>>>> >
>>>>
>>>> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/8fcd5cba7938/src/share/vm/services/nmtDCmd.cpp#l156
>>>>
>>>> > >>
>>>> > >
>>>> > > Sure but that is more of a debugging check - we
>>>> never expect
>>>> > to reach
>>>> > > that code.
>>>> > >
>>>> > > Unfortunately I do see some other implicit aborts
>>>> due to
>>>> > allocation
>>>> > > failures, but I have to say this seems very wrong to
>>>> me.
>>>> > >
>>>> > > David
>>>> > > -----
>>>> > >
>>>> > >> If target VM is aborted, caller (e.g. jcmd) cannot
>>>> receive
>>>> > response. I
>>>> > >> think that the caller show Exception.
>>>> > >>
>>>> > >> Thanks,
>>>> > >>
>>>> > >> Yasumasa
>>>> > >>
>>>> > >> 2016/01/20 7:37 "David Holmes"
>>>> <david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>
>>>> > <mailto:david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>>
>>>> > >> <mailto:david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>
>>>> > <mailto:david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>>>>:
>>>> > >>
>>>> > >> On 19/01/2016 11:19 PM, Yasumasa Suenaga wrote:
>>>> > >>
>>>> > >> Hi,
>>>> > >>
>>>> > >> I uploaded a new webrev:
>>>> > >>
>>>> >
>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8147388/webrev.03/
>>>> > >>
>>>> > >> It is malloc/free version.
>>>> > >> If NULL returns from malloc(), it calls
>>>> > vm_exit_out_of_memory().
>>>> > >>
>>>> > >>
>>>> > >> That seems rather extreme - do other dcmd failures
>>>> abort
>>>> > the VM? I
>>>> > >> would have expected some kind of error
>>>> propagation back
>>>> > to the
>>>> > >> caller.
>>>> > >>
>>>> > >> Thanks,
>>>> > >> David
>>>> > >>
>>>> > >> All test about this changes work fine.
>>>> > >> Please review.
>>>> > >>
>>>> > >> Thanks,
>>>> > >>
>>>> > >> Yasumasa
>>>> > >>
>>>> > >>
>>>> > >> On 2016/01/19 22:07, Dmitry Samersoff wrote:
>>>> > >>
>>>> > >> David,
>>>> > >>
>>>> > >> Anytime we start to use a language
>>>> feature
>>>> > for the first
>>>> > >> time we need
>>>> > >> to be extra diligent to make sure
>>>> there are
>>>> > no unintended
>>>> > >> side-effects and that all our
>>>> supported
>>>> > compilers (and
>>>> > >> probably a few
>>>> > >> others used in the community) do
>>>> the right
>>>> > thing. A bit
>>>> > >> of googling
>>>> > >> seems to indicate that variable length
>>>> arrays
>>>> > are part
>>>> > >> of C99 but are
>>>> > >> not allowed in C++ - though gcc has an
>>>> > extension that
>>>> > >> does allow
>>>> > >> them.
>>>> > >>
>>>> > >>
>>>> > >> I hear your concern.
>>>> > >>
>>>> > >> Moreover I revisit my advice and think
>>>> it's
>>>> not a
>>>> > good idea
>>>> > >> to do a
>>>> > >> stack allocation based on unverified user
>>>> input.
>>>> > >>
>>>> > >> Yasumasa, sorry for leading in wrong
>>>> direction.
>>>> > Lets use
>>>> > >> malloc/free in
>>>> > >> this case.
>>>> > >>
>>>> > >> Nevertheless, I would like to start
>>>> broader
>>>> > evaluation of
>>>> > >> possible use
>>>> > >> on-stack allocation (either alloca or
>>>> VLA) -
>>>> > hotspot may
>>>> > >> benefit of it
>>>> > >> in many places.
>>>> > >>
>>>> > >> -Dmitry
>>>> > >>
>>>> > >> On 2016-01-19 02:06, David Holmes wrote:
>>>> > >>
>>>> > >> On 19/01/2016 7:26 AM, Dmitry
>>>> Samersoff
>>>> wrote:
>>>> > >>
>>>> > >> David,
>>>> > >>
>>>> > >> On 2016-01-18 23:47, David Holmes
>>>> wrote:
>>>> > >>
>>>> > >> On 18/01/2016 11:20 PM, Dmitry
>>>> > Samersoff wrote:
>>>> > >>
>>>> > >> Yasumasa,
>>>> > >>
>>>> > >> Can we use VLA
>>>> (Variable
>>>> > Length Arrays) ?
>>>> > >>
>>>> > >>
>>>> > >> Apple LLVM version 6.1.0
>>>> > (clang-602.0.53)
>>>> > >> (based on LLVM
>>>> > >> 3.6.0svn) Target:
>>>> > x86_64-apple-darwin14.5.0
>>>> > >> Thread model:
>>>> > >> posix
>>>> > >>
>>>> > >> Compiles it just fine.
>>>> > >>
>>>> > >>
>>>> > >> Are we using variable
>>>> length arrays
>>>> > anywhere
>>>> > >> else in the VM yet?
>>>> > >>
>>>> > >>
>>>> > >> Probably not.
>>>> > >>
>>>> > >> But personally, I see no reason to
>>>> don't
>>>> > use it for
>>>> > >> simple cases
>>>> > >> like this one.
>>>> > >>
>>>> > >>
>>>> > >> Anytime we start to use a language
>>>> feature
>>>> > for the first
>>>> > >> time we need
>>>> > >> to be extra diligent to make sure
>>>> there are
>>>> > no unintended
>>>> > >> side-effects and that all our
>>>> supported
>>>> > compilers (and
>>>> > >> probably a few
>>>> > >> others used in the community) do
>>>> the right
>>>> > thing. A bit
>>>> > >> of googling
>>>> > >> seems to indicate that variable length
>>>> arrays
>>>> > are part
>>>> > >> of C99 but are
>>>> > >> not allowed in C++ - though gcc has an
>>>> > extension that
>>>> > >> does allow
>>>> > >> them.
>>>> > >>
>>>> > >> This reports they are not available in
>>>> Visual
>>>> > Studio C++:
>>>> > >>
>>>> > >>
>>>> > https://msdn.microsoft.com/en-us/library/zb1574zs.aspx
>>>> > >>
>>>> > >> David -----
>>>> > >>
>>>> > >> What are the implications for
>>>> > allocation and in
>>>> > >> particular
>>>> > >> allocation failure?
>>>> > >>
>>>> > >>
>>>> > >> This allocation just reserves some
>>>> space
>>>> > on the
>>>> > >> stack[1], so it
>>>> > >> can cause stack overflow if we
>>>> attempt to
>>>> > allocate
>>>> > >> two much bytes.
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >> 1. Listing fragment (extra
>>>> labels are
>>>> > removed)
>>>> > >>
>>>> > >> 3 .Ltext0: 5
>>>> > >> .LC0:
>>>> > >>
>>>> > >> 14:test.cxx **** void
>>>> testme(int n) {
>>>> > >> 15:test.cxx ****
>>>> > >> char m[n]; 25 0000 4863FF
>>>> > movslq
>>>> > >> %edi, %rdi 28 0003
>>>> > >> 55 pushq
>>>> %rbp 37
>>>> > 0004 BE000000
>>>> > >> movl $.LC0, %esi 41 0009
>>>> 4883C70F
>>>> > >> addq $15,
>>>> > >> %rdi 46 000d
>>>> 31C0 xorl
>>>> > %eax,
>>>> > >> %eax 50 000f
>>>> > >> 4883E7F0 andq
>>>> $-16,
>>>> %rdi
>>>> > 54 0013
>>>> > >> 4889E5
>>>> > >> movq %rsp, %rbp 59 0016 4829FC
>>>> > >> subq %rdi,
>>>> > >> %rsp 64 0019
>>>> BF010000 movl
>>>> > $1, %edi
>>>> > >> 65 001e 4889E2
>>>> > >> movq %rsp, %rdx 66 0021
>>>> E8000000
>>>> > call
>>>> > >> __printf_chk 16:test.cxx ****
>>>> > printf("%s",
>>>> > >> m); 17:test.cxx
>>>> > >> **** }
>>>> > >>
>>>> > >> -Dmitry
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >> David -----
>>>> > >>
>>>> > >> -Dmitry
>>>> > >>
>>>> > >> On 2016-01-18 16:09,
>>>> Yasumasa
>>>> > Suenaga wrote:
>>>> > >>
>>>> > >> Hi Dmitry,
>>>> > >>
>>>> > >> 1. It might be
>>>> better to
>>>> > have one
>>>> > >> jcmd to run both
>>>> java and
>>>> > >> native java
>>>> agent. If
>>>> > agent library
>>>> > >> name ends with
>>>> ".jar"
>>>> > >> we can assume
>>>> it's java
>>>> > agent.
>>>> > >>
>>>> > >>
>>>> > >> Okay, I'll try it.
>>>> > >>
>>>> > >> if
>>>> (_libpath.value() ==
>>>> > NULL) {
>>>> > >> error ... }
>>>> > >>
>>>> > >>
>>>> > >> I will add it.
>>>> However, I
>>>> > note you that
>>>> > >> _libpath is given
>>>> > >> mandatory flag.
>>>> > >>
>>>> > >>
>>>> >
>>>>
>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-January/018661.html
>>>>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >> char options[option_len];
>>>> > >>
>>>> > >>
>>>> > >> Can we use VLA
>>>> (Variable
>>>> > Length Arrays)
>>>> > >> ? I'm worried that
>>>> > >> several C++
>>>> compiler cannot
>>>> > compile this
>>>> > >> code.
>>>> > >>
>>>> > >> http://clang.llvm.org/compatibility.html#vla
>>>> > >>
>>>> > >>
>>>> > >> Thanks,
>>>> > >>
>>>> > >> Yasumasa
>>>> > >>
>>>> > >>
>>>> > >> On 2016/01/18
>>>> 19:38, Dmitry
>>>> > Samersoff
>>>> > >> wrote:
>>>> > >>
>>>> > >> Yasumasa,
>>>> > >>
>>>> > >> 1. It might be
>>>> better to
>>>> > have one
>>>> > >> jcmd to run both
>>>> java and
>>>> > >> native java
>>>> agent. If
>>>> > agent library
>>>> > >> name ends with
>>>> ".jar"
>>>> > >> we can assume
>>>> it's java
>>>> > agent.
>>>> > >>
>>>> > >> 2. Please get
>>>> rid of
>>>> > malloc/free and
>>>> > >> check
>>>> _libpath.value()
>>>> > >> for NULL at ll.
>>>> 295 and
>>>> > below.
>>>> > >>
>>>> > >>
>>>> > >> if
>>>> (_libpath.value() ==
>>>> > NULL) {
>>>> > >> error ... }
>>>> > >>
>>>> > >> if
>>>> (_option.value() ==
>>>> > NULL) {
>>>> > >>
>>>> > >> JvmtiExport::load_agent_library("instrument",
>>>> > >> "false",
>>>> > >> _libpath.value(),
>>>> output());
>>>> > >> return; }
>>>> > >>
>>>> > >> size_t
>>>> option_len = \
>>>> > >>
>>>> strlen(_libpath.value()) +
>>>> > >>
>>>> strlen(_option.value()) + 1;
>>>> > >>
>>>> > >> char
>>>> options[option_len];
>>>> > >>
>>>> > >> ....
>>>> > >>
>>>> > >> -Dmitry
>>>> > >>
>>>> > >>
>>>> > >> On 2016-01-15
>>>> 16:33, Yasumasa
>>>> > >> Suenaga wrote:
>>>> > >>
>>>> > >> Hi,
>>>> > >>
>>>> > >> I added
>>>> permissions
>>>> > and tests in
>>>> > >> new webrev:
>>>> > >>
>>>> > >>
>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8147388/webrev.01/
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >> Two tests (LoadAgentDcmdTest,
>>>> > LoadJavaAgentDcmdTest) work
>>>> > >> fine.
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >> Thanks,
>>>> > >>
>>>> > >> Yasumasa
>>>> > >>
>>>> > >>
>>>> > >> On 2016/01/15
>>>> 17:20,
>>>> > Staffan
>>>> > >> Larsen wrote:
>>>> > >>
>>>> > >> This is
>>>> a good
>>>> > improvement
>>>> > >> overall.
>>>> > >>
>>>> > >> The new
>>>> > diagnostic commands
>>>> > >> need to
>>>> have proper
>>>> > >>
>>>> permissions
>>>> set:
>>>> > >>
>>>> > >> static
>>>> const
>>>> > JavaPermission
>>>> > >>
>>>> permission() {
>>>> > >>
>>>> JavaPermission p =
>>>> > >>
>>>> > >> {"java.lang.management.ManagementPermission",
>>>> > >> “control",
>>>> NULL};
>>>> > return p; }
>>>> > >>
>>>> > >> And as
>>>> David
>>>> > said: tests! See
>>>> > >>
>>>> > >> hotspot/test/serviceability/dcmd/jvmti.
>>>> > >>
>>>> > >> Thanks,
>>>> /Staffan
>>>> > >>
>>>> > >> On
>>>> 14 jan.
>>>> > 2016, at
>>>> > >> 15:00,
>>>> > Yasumasa Suenaga
>>>> > >>
>>>> > <yasuenag at gmail.com <mailto:yasuenag at gmail.com>
>>>> <mailto:yasuenag at gmail.com <mailto:yasuenag at gmail.com>>
>>>> > >>
>>>> > >> <mailto:yasuenag at gmail.com <mailto:yasuenag at gmail.com>
>>>> <mailto:yasuenag at gmail.com <mailto:yasuenag at gmail.com>>>>
>>>> > >> wrote:
>>>> > >>
>>>> > >> Hi
>>>> all,
>>>> > >>
>>>> > >> We
>>>> can use
>>>> > Attach API to
>>>> > >> attach
>>>> JVMTI
>>>> > agent to
>>>> > >> live
>>>> > >>
>>>> process.
>>>> > However, we
>>>> > >>
>>>> have to
>>>> write
>>>> > Java code
>>>> > >> for
>>>> it.
>>>> > >>
>>>> > >> If
>>>> we can
>>>> > attach JVMTI
>>>> > >> agents
>>>> > through jcmd,
>>>> > >> it is
>>>> > >> very
>>>> useful.
>>>> > So I want
>>>> > >> to
>>>> add two
>>>> > new diagnostic
>>>> > >>
>>>> commands:
>>>> > >>
>>>> > >> *
>>>> > JVMTI.agent_load: Load
>>>> > >> JVMTI
>>>> native
>>>> > agent. *
>>>> > >>
>>>> > JVMTI.javaagent_load:
>>>> > >>
>>>> Load JVMTI
>>>> > java agent.
>>>> > >>
>>>> > >> I
>>>> maintain
>>>> > two JVMTI
>>>> > >>
>>>> agents -
>>>> > HeapStats [1]
>>>> > >> and
>>>> > >>
>>>> JLivePatcher
>>>> > [2]. [1] is
>>>> > >> native
>>>> agent,
>>>> > [2] is java
>>>> > >>
>>>> agent. They
>>>> > provide a
>>>> > >>
>>>> program for
>>>> > attaching to
>>>> > >> live
>>>> > >>
>>>> process.
>>>> > >>
>>>> > >> I
>>>> guess
>>>> that
>>>> > various
>>>> > >> JVMTI
>>>> agents
>>>> > provide own
>>>> > >> attach
>>>> > >>
>>>> mechanism
>>>> > like them. I
>>>> > >> think
>>>> that we
>>>> > should
>>>> > >> provide
>>>> > >>
>>>> general way
>>>> > to attach.
>>>> > >>
>>>> > >> I've
>>>> uploaded
>>>> > webrev.
>>>> > >>
>>>> Could you
>>>> > review it?
>>>> > >>
>>>> > >>
>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8147388/webrev.00/
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >> I'm jdk9 committer, however I cannot
>>>> access
>>>> JPRT.
>>>> > >>
>>>> > >> So
>>>> I need a
>>>> > sponsor.
>>>> > >>
>>>> > >>
>>>> > >>
>>>> Thanks,
>>>> > >>
>>>> > >>
>>>> Yasumasa
>>>> > >>
>>>> > >>
>>>> > >> [1]
>>>> > >>
>>>> > >> http://icedtea.classpath.org/wiki/HeapStats
>>>> > >> [2]
>>>> > >>
>>>> > >> https://github.com/YaSuenag/jlivepatcher
>>>> > >> (in
>>>> > >>
>>>> Japanese)
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> > >>
>>>> >
>>>> >
>>>> > --
>>>> > Dmitry Samersoff
>>>> > Oracle Java development team, Saint Petersburg, Russia
>>>> > * I would love to change the world, but they won't
>>>> give me the
>>>> > sources.
>>>> >
>>>> >
>>>>
>>>>
>>>> --
>>>> Dmitry Samersoff
>>>> Oracle Java development team, Saint Petersburg, Russia
>>>> * I would love to change the world, but they won't give me the
>>>> sources.
>>>>
>>>
>>>
>
>
More information about the serviceability-dev
mailing list