RFR(M): 8140482: Various minor code improvements (runtime)
Markus Gronlund
markus.gronlund at oracle.com
Thu Nov 5 10:15:10 UTC 2015
Hi Goetz,
Thanks for suggesting these fixes.
Also thanks for pointing to the issue with _NSIG and MAXSIGNUM - looks like MAXSIGNUM is defined all over the place (platform specific + platform specific jsig's)...
I also think it makes perfect sense to bound the signals to the dimensions of the arrays where they will be stored (sigact[MAXSIGNUM] and sigflags[MAXSIGNUM]).
SR_initialize() will (after optionally fetching the env variable for SR_signum) eventually call into:
void os::Linux::set_our_sigflags() {
assert(sig > 0 && sig < MAXSIGNUM, "vm signal out of expected range");
sigflags[sig] = flags;
}
This assert makes me question the expression in SR_initialize():
if (sig > 0 || sig < _NSIG) { <<----
SR_signum = sig;
}
Due to shortcutting there is no upper bound range check on the signal here. If _NSIG is larger than MAXSIGNUM this could be a significant problem.
Should most likely be changed to:
(sig > 0 && sig < MAXSIGNUM)
Thanks
Markus
-----Original Message-----
From: David Holmes
Sent: den 4 november 2015 10:29
To: Lindenmaier, Goetz; hotspot-runtime-dev at openjdk.java.net; serviceability-dev
Subject: Re: RFR(M): 8140482: Various minor code improvements (runtime)
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,.
>>>>>>>
More information about the hotspot-runtime-dev
mailing list