Goetz, > Should I remove the _NSIG issue from this change and > open an issue of it's own discussed on the serviceability list?
Yes please. -Dmitry On 2015-11-04 11:01, 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. > > Should I remove the _NSIG issue from this change and > open an issue of it's own discussed on the serviceability list? > > Best regards, > Goetz. > > >> -----Original Message----- >> From: David Holmes [mailto:[email protected]] >> Sent: Mittwoch, 4. November 2015 08:15 >> To: Lindenmaier, Goetz; [email protected]; >> 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:[email protected]] >>>> Sent: Dienstag, 3. November 2015 11:07 >>>> To: Lindenmaier, Goetz; [email protected]; >>>> 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,. >>>>>>> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
