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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Wed Nov 4 07:32:41 UTC 2015

Hi Coleen,

thanks for looking at this, and thanks for sponsoring! I'll ping you once the _NSIG issue David want's to have addressed is cleared.

Best regards, Goetz.

-----Original Message----- From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-_ _bounces at openjdk.java.net] On Behalf Of Coleen Phillimore Sent: Mittwoch, 4. November 2015 00:34 To: hotspot-runtime-dev at openjdk.java.net Subject: Re: RFR(M): 8140482: Various minor code improvements (runtime)

This looks good to me. If you send me an hg export output, I'll sponsor it. thanks! Coleen On 11/3/15 9: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. > > 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