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

Dmitry Samersoff dmitry.samersoff at oracle.com
Thu Nov 5 11:07:09 UTC 2015

Goetz,

Sorry for being later:

  1. libproc_impl.c

it might be better to check that strlen(alt_root) + strlen(name) is less than PATH_MAX at ll. 50 and return an error. Then we can safely leave strcat staff as it is.

  1. ps_core.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 exec_php->p_filesz > BUF_SIZE and return an error immediately.

Also, please add a comment that BUF_SIZE is (PATH_MAX + NAME_MAX + 1)

  1. os_linux.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

5944:

What is the reason of the change? core_pattern is initialized to zero at ll. 5939.

It might be better to check that ret is less than corep_pattern_len and then use ret instead of strlen at ll. 5948

deoptimization.cpp:

2085, 2182:

jio_snprintf always return -1 and always null-terminate string in case of overflow. So original code is incorrect and we can just remove it.

attachListener.hpp:

125,143:

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

heapDumper.cpp:

1983:

we checked that HeapDumpPath fits in the buffer at ll 1973, so we can safely use strcpy() here.

xmlstream.cpp:

354:

What is the reason of this change? we checked that string fits to buffer at ll 348.

-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

More information about the hotspot-runtime-dev mailing list