RFR(M): 8140482: Various minor code improvements (runtime) (original) (raw)

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Mon Nov 9 20:29:49 UTC 2015

Hi Goetz,

I'll push the fix.

Thanks, Serguei

On 11/9/15 06:11, Lindenmaier, Goetz wrote:

Hi David, Dmitry and Serguei,

I think this is well reviewed now. Coleen had volunteered to sponsor this, but she is off currently I think. Could one of you be so friendly to sponsor this change? Thanks a lot, Goetz.

-----Original Message----- From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com] Sent: Montag, 9. November 2015 11:25 To: Lindenmaier, Goetz; 'hotspot-runtime-dev at openjdk.java.net' Subject: Re: RFR(M): 8140482: Various minor code improvements (runtime)

Goetz, Thumbs up. -Dmitry On 2015-11-09 13:21, Lindenmaier, Goetz wrote: Oh, sure, this is to be removed. I updated the webrev in-place. http://cr.openjdk.java.net/~goetz/webrevs/8140482-covRt/webrev.04/

Best regards, Goetz.

-----Original Message----- From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com] Sent: Freitag, 6. November 2015 18:56 To: Lindenmaier, Goetz; 'hotspot-runtime-dev at openjdk.java.net' Subject: Re: RFR(M): 8140482: Various minor code improvements (runtime) Goetz, Looks good for me except one small nit: oslinux.cpp: 5937 is not necessary. -Dmitry On 2015-11-06 19:18, Lindenmaier, Goetz wrote: Hi, Thanks for looking at this again! New webrev: http://cr.openjdk.java.net/~goetz/webrevs/8140482-covRt/webrev.04/ I copied out the remaining issues, Hope this makes the mail better readable: 2. pscore.c If you don't trust ELF header, it might be better to write 822 interpname[execphp->pfilesz] = '\0'; Done. I figured the function is indented by 3, I fixed that, too. oslinux.cpp It might be better to further refactor this code to avoid scanner complains. Yes that's a good idea. I also return early if the file was not opened, and only do corepattern[ret] = '\0'; in the else branch. xmlstream.cpp:

354: Could you add int formatlen == strlen(format); and then use memcpy and calculated offsets/sizes here instead of strn* ? I optimized it. One strlen is sufficient. I also ran another scan, which is fine. Best regards, Goetz.

-----Original Message----- From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com] Sent: Freitag, 6. November 2015 11:27 To: Lindenmaier, Goetz; 'hotspot-runtime-dev at openjdk.java.net' Subject: Re: RFR(M): 8140482: Various minor code improvements (runtime) Goetz, Thank you for this work. Please, see below inline. On 2015-11-06 00:36, Lindenmaier, Goetz wrote: Hi Dmitry, Finally I have a new webrev: http://cr.openjdk.java.net/~goetz/webrevs/8140482- covRt/webrev.03/ I ran the scan again, twice, which take a while. See coments below. 1. libprocimpl.c it might be better to check that strlen(altroot) + strlen(name) is less than PATHMAX at ll. 50 and return an error. Then we can safely leave strcat staff as it is. Yes, that would look better, but the scan does not grok it. It still complains: "You might overrun the 4097 byte fixed-size string altpath by copying s without checking the length." I would like to keep my changes. OK. 2. pscore.c if interpreter name is truncated for some reason, we have no chance to open it and can't continue. So it might be better to check if execphp->pfilesz > BUFSIZE and return an error immediately. Also, please add a comment that BUFSIZE is (PATHMAX + NAMEMAX + 1) I added what you asked for. But that does not solve my issue. If there is no nul in ph->core->execfd the code fails. So I also left my code in there. If you don't trust ELF header, it might be better to write 822 interpname[execphp->pfilesz] = '\0'; 3. oslinux.cpp 4257: I'm OK with this change but we should revisit the use of MAXSIGNUM under Linux and probably define it to NSIG for the rest of VM I opened an extra issue for this (for the change in SRinitialize() I removed.) https://bugs.openjdk.java.net/browse/JDK-8141529 OK 5944: What is the reason of the change? corepattern is initialized to zero at ll. 5939. If the file is corrupted, there might be more than 128 chars in it. It might be better to check that ret is less than coreppatternlen and then use ret instead of strlen at ll. 5948 That's a good point. It also saves the cost of strlen(). I now return if too many chars were read. But with removing writing '0' the scan now complains, again: "stringnullargument: Function read does not terminate string *corepattern.", And further down at the next strlen: "Passing unterminated string corepattern to strlen, which expects a null-terminated string." I'd like to add the '0' again. It might be better to further refactor this code to avoid scanner complains. 5937: if (ret <= 0 || ret >= corepatternlen || *corepattern == '\n') { return -1; } after that ifs at 5942 and 5991 is not necessary and we can make scanner happy as 5943 corepattern[ret] = '\0'; if (corepattern[ret-1] == '\n') corepattern[ret-1] = '\0' deoptimization.cpp: 2085, 2182: jiosnprintf always return -1 and always null-terminate string in case of overflow. So original code is incorrect and we can just remove it. Good point. I think I fixed jiosnprintf at some point :) Should have known. OK. PS: Be careful fixing jiosnprintf because posix versions of vsnprintf returns the number of charĐ°cters to be written but windows version returns -1 in case of overflow Correct solution that uses vsnptrinf(NULL, 0 ... ) and vscprintf to check overflow explicitly obviously will have performance impact. attachListener.hpp: 125,143: It might be better to calculate strlen(name) once, than use memcpy. Fixed. OK. heapDumper.cpp: 1983: we checked that HeapDumpPath fits in the buffer at ll 1973, so we can safely use strcpy() here. I had removed my fix as David asked for it. I change it to strcpy. OK. xmlstream.cpp: 354: What is the reason of this change? we checked that string fits to buffer at ll 348. A guarantee can be overruled with SuppressErrorAt. Also in the opt build. So the code is also reachable with bad values. This code is executed in a loop so I would prefer to don't have extra strlen calls here. Could you add int formatlen == strlen(format); and then use memcpy and calculated offsets/sizes here instead of strn* ? -Dmitry Best regards, Goetz.

-Dmitry

On 2015-10-27 10:39, Lindenmaier, Goetz wrote: Hi, SAP requires us to fix a row of issues in hotspot. I'd like to share these with openjdk: http://cr.openjdk.java.net/~goetz/webrevs/8140482- covRt/webrev.00 Please review this change. I please need a sponsor. The fixes in detail: libprocimpl.c: Do strncpy in case getenv returned a bad string. Strcat could overflow the buffer. Use strncat instead. pscore.c: Pread not necessarily terminates interpname which is printed thereafter. Increase buffer size by 1 and add '\0'. stubRoutinesx86.cpp: Cast to proper type. This way, left and right of '&' have the same type. attachListenerlinux.cpp: Read does not terminate buf. Size for '\0' is already considered. oslinux.cpp: Array sigflags[] has size MAXSIGNUM==32. NSIG is bigger than MAXSIGNUM (NSIG == 65 on my machine). sig is checked to be smaller than NSIG. Later, in setoursigflags(), sig is used to access sigflags[MAXSIGNUM] which can overflow the array. Should we also increase MAXSIGNUM? os::getcorepath(): read does not terminate string, but strlen is called on it. The size already foresees one char for the '\0' byte. codeBuffer.cpp: Newcapacity is not initialized. Figureexpandedcapacities() handles this correctly, but initializing this is cheap and safe. dict.cpp: If j-- is executed for j==0, the loop aborts because j is unsigned (0-- = b- cnt). Instead, only do j++ if necessary. generateOopMap.cpp: Idx is read from String. This is only called with constant strings, so compare should be folded away by optimizing compilers if inlined. deoptimization.cpp: If buflen == 0, buf[-1] is accessed. task.cpp: Fatal can return if -XX:SuppressErrorAt is used. Just don't access the array in this case. attachListener.hpp: Do strncpy to not overflow buffer. Don't write more chars than before. heapDumper.cpp: strncpy does not null terminate. Some of these, as the issue in codeBuffer.cpp, are actually handled correctly. Nevertheless this is not that obvious so that somebody changing the code Could oversee he has to add the initialization. Some of these fixes are part of SAP JVM for a long time. This change has been tested with our nightly build of openJDK. Best regards, Goetz,. -- 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. -- 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 hotspot-runtime-dev mailing list