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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Thu Nov 5 10:36:17 UTC 2015

Hi Markus,

yes, what you say is exactly my understanding. _NSIG is larger than MAXSIGNUM! But see also my reply to Serguei.

Thanks for looking at this! Best regards, Goetz

-----Original Message----- From: Markus Gronlund [mailto:markus.gronlund at oracle.com] Sent: Donnerstag, 5. November 2015 11:15 To: Lindenmaier, Goetz Cc: hotspot-runtime-dev at openjdk.java.net; serviceability-dev; David Holmes Subject: RE: RFR(M): 8140482: Various minor code improvements (runtime)

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]). SRinitialize() will (after optionally fetching the env variable for SRsignum) eventually call into: void os::Linux::setoursigflags() { assert(sig > 0 && sig < MAXSIGNUM, "vm signal out of expected range");_ _sigflags[sig] = flags;_ _}_ _This assert makes me question the expression in SRinitialize():_ _if (sig > 0 || sig < NSIG) { <<----_ _SRsignum = 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