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?
Thanks,
Serguei
**
Jeremy
