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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Fri Nov 6 16🔞52 UTC 2015

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 core_pattern[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.

More information about the hotspot-runtime-dev mailing list