RFR(M): 8140482: Various minor code improvements (runtime)
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Nov 5 11:46:03 UTC 2015
Hi Goetz,
The fix looks good.
Thank you for the explanations and separation of the SR_initialize() issue!
Thanks,
Serguei
On 11/5/15 02:49, Lindenmaier, Goetz wrote:
>
> Hi,
>
> Serguei, thanks for looking at the issue!
>
> I read a bit about the _NSIG issue.
>
> _/JAVA_SR/_SIGNUM is documented here:
> http://www.oracle.com/technetwork/java/javase/signals-139944.html.
>
> Linux has 32 predefined signals, and another 32 ‘real time signals’ to
> be used by the user.
>
> http://www.linuxprogrammingblog.com/all-about-linux-signals?page=9
>
> MAXSIGNUM is defined by hotspot to 32.
>
> _NSIG is defined on linux to 65.
>
> I think the current handling of this in SR_initilize() is quite bad.
>
> To avoid overwriting the sigflags array my change to
> set_our_sigflags() is sufficient.
>
> SR_initialize() should be improved in several means.
>
> -The code should check that the signal read is > MAX2(SIGSEGV, SIGBUS)
> (assert is not sufficient)
>
> -The code should check that the signal read is < MIN2(_NSIG, MAXSIGNUM)
>
> -It should warn if it is not. Currently the env var is silently ignored
>
> My current fix makes it impossible to use real time signals for
> _/JAVA_SR/_SIGNUM.
>
> Therefore I think we should increase MAXSIGNUM on linux to 65.
>
> Also, we should check bsd, which might have a similar issue.
>
> I will remove my fix from SR_initialize() and post a webrev of its own
> for
>
> this issue. @Dmitry, I just saw your mail …
>
> In this new webrev I removed the change to SR_initialize() and
>
> fixed the spaces around the ‘+’.:
>
> http://cr.openjdk.java.net/~goetz/webrevs/8140482-covRt/webrev.02/
> <http://cr.openjdk.java.net/%7Egoetz/webrevs/8140482-covRt/webrev.02/>
>
> Best regards,
>
> Goetz.
>
> *From:*serguei.spitsyn at oracle.com [mailto:serguei.spitsyn at oracle.com]
> *Sent:* Donnerstag, 5. November 2015 04:49
> *To:* David Holmes; Lindenmaier, Goetz;
> hotspot-runtime-dev at openjdk.java.net; serviceability-dev
> *Subject:* Re: RFR(M): 8140482: Various minor code improvements (runtime)
>
> Hi Goetz,
>
> The fix looks good.
> Thanks for the improvements!
> The _NSIG related fix looks Ok to me but I do not feel myself
> qualified to make a final decision.
>
> A couple of minor comments:
>
> *src/share/vm/libadt/dict.cpp*
>
> 149 nb->_keyvals[nbcnt + nbcnt + 1] = b->_keyvals[j+j+1];
> 152 b->_keyvals[j+j] = b->_keyvals[b->_cnt + b->_cnt];
> 153 b->_keyvals[j+j+1] = b->_keyvals[b->_cnt + b->_cnt + 1];
>
> Need spaces around the '+' sign for completeness.
>
>
> *src/os/linux/vm/attachListener_linux.cpp*
>
> 258 buf[max_len-1] = '\0';
>
> Need spaces around the '-' sign.
>
>
> *src/share/vm/services/attachListener.hpp*
>
> 126 strncpy(_name, name, MIN2(strlen(name)+1,
> (size_t)name_length_max));
> 143 strncpy(_arg[i], arg, MIN2(strlen(arg)+1,
> (size_t)arg_length_max));
>
>
> Need spaces around the '+' sign.
>
>
> *agent/src/os/linux/ps_core.c*
>
> 815 char interp_name[BUF_SIZE+1];
>
> Need spaces around the '+' sign.
>
>
> Thanks,
> Serguei
>
>
> On 11/4/15 01:29, David Holmes wrote:
>
> 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
> <mailto: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/
> <http://cr.openjdk.java.net/%7Egoetz/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
> <mailto: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