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

[Lindenmaier, Goetz]

Hi Dmitry,

Finally I have a new webrev:
http://cr.openjdk.java.net/~goetz/webrevs/8140482-covRt/webrev.03/
I ran the scan again, twice, which take a while.  See coments below.

> 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.
Yes, that would look better, but the scan does not grok it. It still
complains: "You might overrun the 4097 byte fixed-size string alt_path by copying s without checking the length."
I would like to keep my changes.

> 2. 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)
I added what you asked for.  But that does not solve my issue. If there is 
no nul in ph->core->exec_fd the code fails.  So I also left my code in there.

> 3. 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
I opened an extra issue for this (for the change in SR_initialize() I removed.)
https://bugs.openjdk.java.net/browse/JDK-8141529

> 5944:
> 
> What is the reason of the change? core_pattern is initialized to zero at
> ll. 5939.
If the file is corrupted, there might be more than 128 chars in it.
 
> It might be better to check that ret is less than corep_pattern_len and
> then use ret instead of strlen at ll. 5948
That's a good point.  It also saves the cost of strlen(). I now return if 
too many chars were read.
But with removing writing \'0' the scan now complains, again: "string_null_argument: Function read does not terminate string *core_pattern.", 
And further down at the next strlen: "Passing unterminated string core_pattern to strlen, which expects a null-terminated string."
I'd like to add the \'0' again.

> 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.
Good point.  I think I fixed jio_snprintf at some point :) Should have known.

> attachListener.hpp:
> 
> 125,143:
> 
> It might be better to calculate strlen(name) once, than use memcpy.
Fixed.

> heapDumper.cpp:
> 
> 1983:
> 
>    we checked that HeapDumpPath fits in the buffer at ll 1973, so we can
> safely use strcpy() here.
I had removed my fix as David asked for it. I change it to strcpy.

> xmlstream.cpp:
> 
> 354:
> 
>    What is the reason of this change? we checked that string fits to
> buffer at ll 348.
A guarantee can be overruled with SuppressErrorAt. Also in the opt build.
So the code is also reachable with bad values.

Best regards,
  Goetz.


> 
> -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:
> >
> > libproc_impl.c:
> > Do strncpy in case getenv returned a bad string.
> > Strcat could overflow the buffer. Use strncat instead.
> >
> > ps_core.c:
> > Pread not necessarily terminates interp_name which is printed thereafter.
> > Increase buffer size by 1 and add '\0'.
> >
> > stubRoutines_x86.cpp:
> > Cast to proper type. This way, left and right of '&' have the same type.
> >
> > attachListener_linux.cpp:
> > Read does not terminate buf. Size for '\0' is already considered.
> >
> > 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?
> > 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.
> >
> > codeBuffer.cpp:
> > New_capacity is not initialized. Figure_expanded_capacities() 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
> * I would love to change the world, but they won't give me the sources.