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

[Lindenmaier, Goetz]

Hi Coleen,

thanks for looking at this, and thanks for sponsoring!
I'll ping you once the _NSIG issue David want's to have
addressed is cleared.

Best regards,
  Goetz.


> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> bounces at openjdk.java.net] On Behalf Of Coleen Phillimore
> Sent: Mittwoch, 4. November 2015 00:34
> To: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(M): 8140482: Various minor code improvements (runtime)
> 
> 
> This looks good to me.  If you send me an hg export output, I'll sponsor it.
> thanks!
> Coleen
> 
> On 11/3/15 9:10 AM, Lindenmaier, Goetz wrote:
> > Hi David,
> >
> > the new scan is already through. I made a new webrev:
> > http://cr.openjdk.java.net/~goetz/webrevs/8140482-covRt/webrev.01/
> >
> > attachListener_linux.cpp:
> > I had moved the string termination out of the loop, and read one
> > less char from the file. The scan still claims "Passing unterminated
> > string buf to strlen" in line 274.  So I will undo it again for the
> > webrev.
> >
> > codeBuffer.cpp
> > Doing memset is fine.  I'll use memset().
> >
> >>>>> 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?
> >>> As I understand, pread does not null-terminate what it read.  So the
> >>> null must come from the file.  This protects against a corrupted file.
> >> So are you saying the nul is not present in the file? I'm not familiar
> >> with the ELF format.
> > There should be a nul in the file, else the code would fail more
> > obviously.  The problem is if the file is corrupted.
> >
> > Best regards,
> >    Goetz.
> >
> >
> >> -----Original Message-----
> >> From: David Holmes [mailto:david.holmes at oracle.com]
> >> Sent: Dienstag, 3. November 2015 11:07
> >> To: Lindenmaier, Goetz; hotspot-runtime-dev at openjdk.java.net;
> >> serviceability-dev
> >> Subject: Re: RFR(M): 8140482: Various minor code improvements
> (runtime)
> >>
> >> Hi Goetz,
> >>
> >> Quick follow up on a couple of things ...
> >>
> >> On 3/11/2015 7:33 PM, Lindenmaier, Goetz wrote:
> >>> Hi David,
> >>>
> >>>> Sorry, lots of folks very busy at the moment as we try to get features
> >>>> finalized before the deadline.
> >>> thanks for looking at this!  I know the Dec. 10 deadline, but I guess that
> also
> >>> holds for us ... at least J1 is over now.  (Unfortunately we could not
> attend
> >>> this year.)
> >> Me neither :)
> >>
> >>>>> 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?
> >>> As I understand, pread does not null-terminate what it read.  So the
> >>> null must come from the file.  This protects against a corrupted file.
> >> So are you saying the nul is not present in the file? I'm not familiar
> >> with the ELF format.
> >>
> >>>>> 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.
> >>> What you propose did not work.  It was my first fix, too.
> >> Hmm okay. The result of the shift must be an unsigned type and the
> >> constant 1 is signed, so needs the cast (or use the unsigned constant
> >> form - 1ud? )
> >>
> >>>>> 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.
> >>> I'll try to move it out of the loop.  Better: I'll check whether the
> >>> scan groks it if I move it out of the loop :)
> >>>
> >>>>> 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.
> >>> I'm also not sure how to best handle this. Might even require a fix
> >>> exceeding this change.  But I think this is the best finding.
> >>>
> >>>>> 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?
> >>> I guess it would work with memset, too.  But I thought the 3-deep loop
> >>> will be unrolled completely so that only three stores remain.
> >> I tend not to try and imagine what the compiler may or may not do. Happy
> >> to take other opinions. Though again I'd prefer if the checker could be
> >> shown that there is no missing initialization.
> >>
> >>>>> 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.
> >>> Yes, the scan does not understand there is j++ right after j-- because
> >>> of the loop iteration.  I saw it complaining about this pattern several
> times.
> >>>
> >>>> Lots of spacing changes in that code make it hard to see the real
> changes.
> >>> Before, I was asked to fix indentation issues in a function I touch.
> >>> Does that only hold for compiler files?
> >> Yes/no/maybe :) Fixing up bad formatting when you are touching an area
> >> can be convenient, however it can also obscure the real changes, so it
> >> depends on the ratio of functional changes to format 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.
> >>> Didn't mean to leave this in here. Removed.
> >>>
> >>>>     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.
> >>> Fixed.
> >>>
> >>>>> 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.
> >>> OK, removed.
> >>>
> >>>>> 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.
> >>> Ok, I added the assert.  As this isn't critical code, I would like to leave the
> >>> check in there, still.
> >>>
> >>>>> 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.
> >>> Hmm, the command comes from outside of the VM.  It's not checked
> >>> very thoroughly, see, e.g., attachListener_windows.cpp:194.  Arg0 is
> >>> checked twice, arg1 and arg2 are not checked at all.
> >> The libattach code is still part of our codebase so should be doing the
> >> right things. The linux and solaris code seems to be doing the expected
> >> name length check. On Windows the name is set using cmd, which is also
> >> subject to a length check:
> >>
> >>    if (strlen(cmd) > AttachOperation::name_length_max) return
> >> ATTACH_ERROR_ILLEGALARG;
> >>
> >>> I add fixes for attachListener_windows.cpp to this change.
> >>>
> >>>>> 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.
> >>> Ok, removed.
> >>>
> >>>>    > 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.
> >>> I assume the 10 is the space needed for the "_done" plus some waste ...
> >>>
> >>> I'll do another run of the scan.  That takes a day.  I'll post a new webrev
> >> after
> >>> that.
> >> Thanks,
> >> David
> >>
> >>> Thank again for this thorough review,
> >>>     Goetz
> >>>
> >>>
> >>>
> >>>
> >>>>> 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,.
> >>>>>