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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Mon Nov 9 10:21:33 UTC 2015

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.

More information about the hotspot-runtime-dev mailing list