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 <yasue...@gmail.com
> <mailto:yasue...@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.samers...@oracle.com
> <mailto:dmitry.samers...@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.hol...@oracle.com
> <mailto:david.hol...@oracle.com>
> >> <mailto:david.hol...@oracle.com
> <mailto:david.hol...@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
> >>
> <yasue...@gmail.com <mailto:yasue...@gmail.com>
> >>
> >> <mailto:yasue...@gmail.com <mailto:yasue...@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.
