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

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 attachListenerwindows.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/ attachListenerlinux.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().

pscore.c: Pread not necessarily terminates interpname which is printed thereafter. Increase buffer size by 1 and add '\0'.

Given: #define BUFSIZE (PATHMAX + NAMEMAX + 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.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 :) pscore.c: Pread not necessarily terminates interpname which is printed thereafter. Increase buffer size by 1 and add '\0'. Given: #define BUFSIZE (PATHMAX + NAMEMAX + 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. stubRoutinesx86.cpp: Cast to proper type. This way, left and right of '&' have the same type. I think you could just have changed the uint64t to uint32t as applied to the shift rather than casting the 1 to uint32t. 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? ) attachListenerlinux.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 :) oslinux.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 setoursigflags(), 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 JAVASRSIGNUM 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: Newcapacity is not initialized. Figureexpandedcapacities() 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., attachListenerwindows.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::namelengthmax) return ATTACHERRORILLEGALARG; I add fixes for attachListenerwindows.cpp to this change. heapDumper.cpp: strncpy does not null terminate. 1973 if (totallength >= sizeof(basepath)) { totallength 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 totallength <= sizeof(basepath),_ _and totalpath 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