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