RFR: JDK-8147388: Add diagnostic commands to attach JVMTI agent.
Yasumasa Suenaga
yasuenag at gmail.com
Fri Jan 22 08:56:16 UTC 2016
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>:
> 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>>:
> >
> > 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>>:
> >
> > 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>>>:
> > >>
> > >> 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>>>
> > >> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160122/3c8e04ef/attachment-0001.html>
More information about the serviceability-dev
mailing list