RFR: JDK-8147388: Add diagnostic commands to attach JVMTI agent.
Dmitry Samersoff
dmitry.samersoff at oracle.com
Sat Jan 23 17:37:47 UTC 2016
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.
>>>
>>
>>
--
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