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

[David Holmes]

On 4/11/2015 6:01 PM, Lindenmaier, Goetz wrote:
> Hi David,
>
> attachListener.hpp:
> I agree that I can not find another possible issue
> with the strcpy.
> Still I think it's better to have the strncpy, as it would have
> protected against the bug in attachListener_windows.cpp.
> But if you insist I'll just remove it.

I don't insist, but I do prefer to place all the guards at the 
"boundary" of the VM rather than at every level when possible.

> Should I remove the _NSIG issue from this change and
> open an issue of it's own discussed on the serviceability list?

Let's give them a chance to respond. I'll ping them on the hotline ;-)

Thanks,
David

> Best regards,
>    Goetz.
>
>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Mittwoch, 4. November 2015 08:15
>> To: Lindenmaier, Goetz; hotspot-runtime-dev at openjdk.java.net;
>> serviceability-dev
>> Subject: Re: RFR(M): 8140482: Various minor code improvements (runtime)
>>
>> Hi Goetz,
>>
>> On 4/11/2015 12: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.
>>
>> Thanks for clarifying.
>>
>> I'm still unclear why the attachListener.hpp changes are needed if we
>> have validated the entry points in the attachListener_<os>.cpp files?
>>
>> Also we still need someone from serviceability to comment on the _NSIG
>> issue.
>>
>> Thanks,
>> David
>>
>>
>>> 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,.
>>>>>>>