RFR(M): 8206977: Minor improvements of runtime code.

[David Holmes]

Hi Goetz,

On 10/07/2018 8:53 PM, Lindenmaier, Goetz wrote:
> Hi,
> 
> I ran coverity on the jdk11 hotspot sources and want to propose the
> following fixes to the runtime code. I scanned the linux x86_64 build.
> Some issues are similar to previous parfait fixes (check for NULL, add
> guarantees etc.) I also identified some issues I consider real problems.
> http://cr.openjdk.java.net/~goetz/wr18/8206977-covRuntime/01/

It will take a while to go through these. I see some false positives 
caused by too local an examination - which is typical of code checkers. 
For example in os_linux.cpp

+     if (buflen > 7) {

we know the buffer coming in is O_BUFLEN in size. Add an assert if you 
like but no need for a hard-wired guard.

I see some asserts changed to guarantees which is unnecessary in general 
- but again appeases static checkers looking at product builds.

I also don't see this as a P3 bug, as there seems only 1 potential real 
bug there (you yourself call these "minor improvements"). So this seems 
unsuitable for JDK 11 now we are in RDP1. But fine for 12.

Will try to go through in more detail tomorrow, but it is somewhat 
tedious to have to work through these in detail to refute the code 
checkers claims of incorrectness.

Thanks,
David
-----


> In detail:
> 
> Real issues:
> ------------
> 
> jvmtiEnvBase.cpp
>    || should be &&.
>    Attention, this is the only change that really will change behaviour.
>    But if thr == NULL we will see a crash below.
> 
> perfMemory_linux.cpp:
>    Wrong buffer length used.
> 
> systemDictionary.cpp:
>    Move code dereferencing ik under if (ik != NULL).
> 
> virtualspace.cpp
>    Initialization is missing. Moved constructor up to the other
>    constructors.
> 
> 
> Useful code improvements:
> -------------------------
> 
> vm_version_ext_x86.cpp
>    Assure buffer is not accessed at offset -1.
> 
> os_linux.cpp
>    Numa_max_node returns int, and a -1 in some cases.
> 
> moduleEntry.cpp
>    name might be NULL. Just a fix for tracing.
> 
> systemDictionaryShared.cpp
>    clearify code.
>    It would be wrong if only entry == NULL would hold, one
>    would hit the assertion below.
> 
> verifier.cpp
>    Fix tracing.
>    Illegal opcode is -1 and should not be passed to name array.
> 
> logOutput.cpp
>    If n_selections == 0, best_selection would be NULL.
>    Move up the assertion and turn into a guarantee.
> 
> filemap.cpp
>    Either base can be NULL, or parts of the code before are dead.
> 
> metaspace.cpp
>    We now an exception is pending.
> 
> klassVtable.cpp
>    Coverity does not like the format in a variable.
>    Anyways this is quite rough coding, transformed to use stringStream
>    as with other similar exceptions.
> 
> jvmFlag.cpp
>    match might be NULL.
> 
> writableFlags.cpp
>    name might be NULL.
> 
> ostream.cpp
>    If ftell returns error code -1, we need not continue.
>    Especially we should not fseek(-1).
> 
> logTestUtils.inline.hpp
>    ftell returns -1.
> 
> test_metachunk.cpp
>    wrong datatype.
>