RFR: JDK-8147388: Add diagnostic commands to attach JVMTI agent.
Dmitry Samersoff
dmitry.samersoff at oracle.com
Fri Jan 22 09:41:55 UTC 2016
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