Wow, go take care of my toddler for a few hours, come back, and all the questions are answered for me! Thanks, Coleen.
To be fair, the original code was actually correct (instead of, you know, implementation-dependent-correct), so I feel a little weird about the whole thing. Jeremy On Wed, Nov 5, 2014 at 9:35 PM, David Holmes <david.hol...@oracle.com> wrote: > On 6/11/2014 3:02 PM, Coleen Phillimore wrote: > >> >> David and Serguei (and Jeremy), see below. Summary: I think Jeremy's >> code and comments are good. >> >> On 11/5/14, 10:11 PM, David Holmes wrote: >> >>> On 6/11/2014 1:00 PM, Coleen Phillimore wrote: >>> >>>> >>>> On 11/5/14, 8:11 PM, serguei.spit...@oracle.com wrote: >>>> >>>>> >>>>> On 11/5/14 4:35 PM, Jeremy Manson wrote: >>>>> >>>>>> >>>>>> >>>>>> On Wed, Nov 5, 2014 at 3:40 PM, Coleen Phillimore >>>>>> <coleen.phillim...@oracle.com <mailto:coleen.phillim...@oracle.com>> >>>>>> wrote: >>>>>> >>>>>> >>>>>> On 11/5/14, 6:13 PM, Jeremy Manson wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On Tue, Nov 4, 2014 at 8:56 PM, serguei.spit...@oracle.com >>>>>>> <mailto:serguei.spit...@oracle.com> <serguei.spit...@oracle.com >>>>>>> <mailto:serguei.spit...@oracle.com>> wrote: >>>>>>> >>>>>>> The fix looks good in general. >>>>>>> >>>>>>> src/share/vm/oops/method.cpp >>>>>>> >>>>>>> 1785 bool contains(Method** m) { >>>>>>> 1786 if (m == NULL) return false; >>>>>>> 1787 for (JNIMethodBlockNode* b = &_head; b != NULL; b >>>>>>> = b->_next) { >>>>>>> 1788 if (b->_methods <= m && m < b->_methods + >>>>>>> b->_number_of_methods) { >>>>>>> *1789 ptrdiff_t idx = m - b->_methods;** >>>>>>> **1790 if (b->_methods + idx == m) {** >>>>>>> 1791 return true; >>>>>>> 1792 }* >>>>>>> 1793 } >>>>>>> 1794 } >>>>>>> 1795 return false; // not found >>>>>>> 1796 } >>>>>>> >>>>>>> >>>>>>> Just noticed that the lines 1789-1792 can be replaced with >>>>>>> one liner: >>>>>>> * return true;* >>>>>>> >>>>>>> >>>>>>> Ah, you have found our crappy workaround for wild pointers to >>>>>>> non-aligned places in the middle of _methods. >>>>>>> >>>>>> >>>>>> Can you explain this? Why are there wild pointers? >>>>>> >>>>>> >>>>>> My belief was that end user code could pass any old garbage to this >>>>>> function. It's called by Method::is_method_id, which is called >>>>>> by jniCheck::validate_jmethod_id. The idea was that this would help >>>>>> check jni deliver useful information in the case of the end user >>>>>> inputting garbage that happened to be in the right memory range. >>>>>> >>>>>> Having said that, at a second glance, it looks as if it that call is >>>>>> protected by a call to is_method() (in checked_resolve_jmethod_id), >>>>>> so the program will probably crash before it gets to this check. >>>>>> >>>>>> The other point about it was that the result of >= and < is >>>>>> technically unspecified; if it were ever implemented as anything >>>>>> other than a binary comparison between integers (which it never is, >>>>>> now that no one has a segmented architecture), the comparison could >>>>>> pass spuriously, so checking would be a good thing. Of course, the >>>>>> comparison could fail spuriously, too. >>>>>> >>>>>> Anyway, I'm happy to leave it in as belt-and-suspenders (and add a >>>>>> comment, obviously, since it has caused confusion), or take it out. >>>>>> Your call. >>>>>> >>>>> >>>>> I'm still confused. >>>>> >>>>> How this code could possibly check anything? >>>>> ptrdiff_t idx = m - b->_methods; >>>>> if (b->_methods + idx == m) { >>>>> >>>>> The condition above always gives true: >>>>> b->_methods + (idx) == b->_methods + (m - b->_methods) == >>>>> (b->_methods- b->_methods) + m == (0 + m) == m >>>>> >>>>> Even if m was unaligned then at the end we compare m with m which is >>>>> still true. >>>>> Do I miss anything? >>>>> >>>> >>>> If 'm' is unaligned we would fail this comparison: >>>> >>>> (gdb) print &methods->_data[2] >>>> $34 = (Method **) 0x7fffe0022440 >>>> (gdb) print &methods->_data[0] >>>> $35 = (Method **) 0x7fffe0022430 >>>> (gdb) print 0x7fffe0022444 - 0x7fffe0022430 >>>> $32 = 20 >>>> >>> >>> I was confused about this too. What we have here is pointer >>> arithmetic, not regular arithmetic, so I'm assuming an unaligned value >>> has to be adjusted before the actual difference is computed. So in >>> practice: >>> >>> m - b->_methods >>> >>> is really >>> >>> adjusted_for_alignment(m) - b->_methods >>> >> >> It's not adjusted for alignment: >> > > Right - now I get it. Pointer difference is an algebraic subtraction with > "div sizeof what is pointed to". For aligned pointers there will be no > remainder and adding back the difference to the initial pointer will yield > the end pointer. But if one of the pointers is not aligned that is not the > case. > > All rather icky. > > Thanks, > David > ---- > > > #include <cstddef> >> >> extern "C" int printf(const char *,...); >> class Method { >> int i ; int j; int k; >> }; >> >> Method* array[10] = { new Method(),new Method(),new Method(),new >> Method(),new Method(),n >> ew Method(),new Method(),new Method(),new Method(),new Method() }; >> >> void test(Method** m) { >> printf("m is 0x%p ", m); >> ptrdiff_t idx = m - array; >> if (array + idx == m) { >> printf("true %ld\n", idx); >> } else { >> printf("false %ld\n", idx); >> } >> } >> main() { >> Method** xx = &array[3]; >> test(xx); >> test((Method**)(((char*)xx) - 2)); >> } >> >> cphilli% a.out >> m is 0x0x601098 true 3 >> m is 0x0x601096 false 2 >> >> >> Coleen >> >> >>> David >>> ----- >>> >>> (gdb) print 20/8 >>>> $33 = 2 >>>> >>>> if m is misaligned 0x7fffe0022444 the idx would be 2 and the expression >>>> (b->_methods + idx) would evaluate to the aligned 0xfffe0022440 so not >>>> equal m. >>>> >>>> But the code could check for misaligned m instead (or it would have >>>> already crashed). I think all bets are off if the address space is >>>> segmented. >>>> >>>> The comment Jeremy added is: >>>> >>>> if (b->_methods <= m && m < b->_methods + >>>> b->_number_of_methods) { >>>> // This is a bit of extra checking, for two reasons. One is >>>> // that contains() deals with pointers that are passed in by >>>> // JNI code, so making sure that the pointer is aligned >>>> // correctly is valuable. The other is that <= and > are >>>> // technically not defined on pointers, so the if guard can >>>> // pass spuriously; no modern compiler is likely to make that >>>> // a problem, though (and if one did, the guard could also >>>> // fail spuriously, which would be bad). >>>> ptrdiff_t idx = m - b->_methods; >>>> if (b->_methods + idx == m) { >>>> return true; >>>> } >>>> >>>> Coleen >>>> >>>>> >>>>> >>>>> Thanks, >>>>> Serguei >>>>> >>>>> ** >>>>> >>>>>> >>>>>> Jeremy >>>>>> >>>>> >>>>> >>>> >>
