Yasumasa, Looks good for me!
-Dmitry On 2016-01-23 17:18, Yasumasa Suenaga wrote: > 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.samers...@oracle.com >>> <mailto: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> >>> > <mailto: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> >>> > <mailto: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>> >>> > >> <mailto: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>> >>> > >> >>> > >> <mailto: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. >>> >> >> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
