RFR: JDK-8147388: Add diagnostic commands to attach JVMTI agent.
Yasumasa Suenaga
yasuenag at gmail.com
Sat Jan 23 14:18:08 UTC 2016
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