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:[email protected]] > Sent: Donnerstag, 5. November 2015 11:15 > To: Lindenmaier, Goetz > Cc: [email protected]; 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]). > > 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; [email protected]; > 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 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:[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,. > >>>>>>>
