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

[David Holmes]

(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:
>
> libproc_impl.c:
> Do strncpy in case getenv returned a bad string.
> Strcat could overflow the buffer. Use strncat instead.

Okay.

> ps_core.c:
> Pread not necessarily terminates interp_name 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?

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

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

> os_linux.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 set_our_sigflags(),
> 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::get_core_path(): read does not terminate string, but strlen is
> called on it.  The size already foresees one char for the '\0' byte.

Ok.

> codeBuffer.cpp:
> New_capacity is not initialized. Figure_expanded_capacities() 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,.
>