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

David Holmes david.holmes at oracle.com
Tue Nov 3 05:05:52 UTC 2015

(adding serviceability-dev as they own SA code and some other parts - eg SR_signal_num issue)

Hi Goetz,

Sorry, lots of folks very busy at the moment as we try to get features finalized before the deadline.

On 27/10/2015 5:39 PM, 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.

Okay.

pscore.c: Pread not necessarily terminates interpname which is printed thereafter. Increase buffer size by 1 and add '\0'.

Given:

#define BUF_SIZE (PATH_MAX + NAME_MAX + 1)

isn't it impossible to encounter that problem?

stubRoutinesx86.cpp: Cast to proper type. This way, left and right of '&' have the same type.

I think you could just have changed the uint64_t to uint32_t as applied to the shift rather than casting the 1 to uint_32t. End result is the same though.

src/cpu/x86/vm/templateTable_x86.cpp

Use of CONST64 seems okay.

attachListenerlinux.cpp: Read does not terminate buf. Size for '\0' is already considered.

Looks a little odd being done on each iteration, but okay I guess.

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?

Need to let the SR folk comment here as something definitely seems wrong, but I'm not 100% sure the what the correct answer is. If _JAVA_SR_SIGNUM is too big it should be validated somewhere and an error or warning reported.

os::getcorepath(): read does not terminate string, but strlen is called on it. The size already foresees one char for the '\0' byte.

Ok.

codeBuffer.cpp: Newcapacity is not initialized. Figureexpandedcapacities() handles this correctly, but initializing this is cheap and safe.

Hmmm ... I hate redundancy - this is pure wasted cycles. If we had to do it would memset not be better? Or would the code-checker not realize what memset was doing?

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.

Not at all obvious to me that it is possible to do j-- when j==0, but the change seems reasonable.

Lots of spacing changes in that code make it hard to see the real changes.

145 // SAPJVM GL j-- can underflow, which will cause the loop to abort.

Seems unnecessary with the code change as noone will understand what j-- you are referring to.

150 nb->_keyvals[nbcnt + nbcnt ] = key; 151 nb->_keyvals[nbcnt + nbcnt + 1] = b->_keyvals[j+j+1];

hotspot-style doesn't align array index expressions like that. Ditto 154/155.

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.

Not a fan of adding conditions that should never be false (hence the assert) and relying on the compiler to elide them.

deoptimization.cpp: If buflen == 0, buf[-1] is accessed.

Okay - but an assert(buflen>0) would be better I think as we should never be calling with a zero-length buffer.

task.cpp: Fatal can return if -XX:SuppressErrorAt is used. Just don't access the array in this case.

Okay. I would not be surprised if we have a lot of potential errors if a fatal/guarantee failure is suppressed.

attachListener.hpp: Do strncpy to not overflow buffer. Don't write more chars than before.

Again we have the assert to catch an error in the caller using an invalid name.

heapDumper.cpp: strncpy does not null terminate.

1973 if (total_length >= sizeof(base_path)) {

total_length already adds +1 for the nul character so the == case is fine AFAICS.

strncpy wont nul-terminate if the string exceeds the buffer size. But we have already established that total_length <= sizeof(base_path), and total_path includes space for a bunch of stuff other than HeapDumpPath, so the strncpy of HeapDumpPath has to copy the nul character.

src/share/vm/services/memoryService.cpp

Ok.

src/share/vm/utilities/xmlstream.cpp

Ok - I'm more concerned about the "magic" 10 in that piece of code.

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.

Not an argument I buy en-masse as it leads to a lot of redundancy through the code paths. Plus these tools that are being run should show if a code changes requires initialization that is not present.

Thanks, David

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,.

More information about the hotspot-runtime-dev mailing list