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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Thu Nov 5 21:36:00 UTC 2015

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 alt_path by copying s without checking the length." I would like to keep my changes.

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->exec_fd the code fails. So I also left my code in there.

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 SR_initialize() I removed.) https://bugs.openjdk.java.net/browse/JDK-8141529

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: "string_null_argument: Function read does not terminate string *core_pattern.", And further down at the next strlen: "Passing unterminated string core_pattern to strlen, which expects a null-terminated string." I'd like to add the '0' again.

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 jio_snprintf at some point :) Should have known.

attachListener.hpp:

125,143: It might be better to calculate strlen(name) once, than use memcpy. Fixed.

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.

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.

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.

More information about the hotspot-runtime-dev mailing list