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.samers...@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 <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. >