United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
Bug ID: JDK-6559052 [OpenJDK] CardTableRS::do_MemRegion() could attempt out-of-bounds translation in addr_for()
JDK-6559052 : [OpenJDK] CardTableRS::do_MemRegion() could attempt out-of-bounds translation in addr_for()

Details
Type:
Bug
Submit Date:
2007-05-17
Status:
Resolved
Updated Date:
2010-04-02
Project Name:
JDK
Resolved Date:
2007-06-20
Component:
hotspot
OS:
generic
Sub-Component:
gc
CPU:
generic
Priority:
P5
Resolution:
Fixed
Affected Versions:
7
Fixed Versions:
hs10 (b14)

Related Reports
Backport:
Backport:

Sub Tasks

Description
Response from ###@###.###:
-------------------------------

...

Thanks. Your fix makes more sense than mine.
Response from ###@###.###:
--------------------------------------

Hi Neo,

Thanks for the heads-up. This seems worth fixing, at least
for hygiene, How about the following instead, where we avoid the
addr_for() translation until we are sure it will be safe,
provided that the mr argument to the method is backed by
card table entries. This avoids the need for the
extra compare-and-branch inside the loop in your patch.

  void do_MemRegion(MemRegion mr) {
    HeapWord* end_of_non_clean = mr.end();
    HeapWord* start_of_non_clean = end_of_non_clean;
    jbyte*       entry = _ct->byte_for(mr.last());
    jbyte* first_entry = _ct->byte_for(mr.start());
    while (entry >= first_entry) {
      jbyte entry_val = *entry;
      HeapWord* cur = _ct->addr_for(entry);
      if (!clear_card(entry)) {
        if (start_of_non_clean < end_of_non_clean) {
          MemRegion mr2(start_of_non_clean, end_of_non_clean);
          _dirty_card_closure->do_MemRegion(mr2);
        }
        end_of_non_clean = cur;
        start_of_non_clean = end_of_non_clean;
      }
      start_of_non_clean = cur;
      entry--;
    }
    if (start_of_non_clean < end_of_non_clean) {
      MemRegion mr2(start_of_non_clean, end_of_non_clean);
      _dirty_card_closure->do_MemRegion(mr2);
    }
  }
Email from ###@###.###: [OpenJDK]
----------------------------
hi,

Acturally, the existing code works well although it misses the
limitation checking, because the current heap layout will make sure
the memory region checking failed before it goes beyond the limitation
of byte map.

The reason is that the nursery space is on the lower address, which
will make its committed card table also on the lower address of inside
the byte maps. While walking through the card table of the mature
space and keeping reduce the entry, we will finally hit the boundary
of its byte map before doing the check of memory region, which will
fire the assert of the addr_for, if it is on the lower address.

So, just adding more safety for this function.

Thanks,
Neo
Index: memory/cardTableRS.cpp
===================================================================
--- memory/cardTableRS.cpp  (revision 86)
+++ memory/cardTableRS.cpp  (working copy)
@@ -163,6 +163,9 @@
    start_of_non_clean = end_of_non_clean;
       }
       entry--;
+      // Adding a limit checking for safety.
+      if (entry < _ct->base_map())
+        break;
       start_of_non_clean = cur;
       cur = _ct->addr_for(entry);
     }
Index: memory/cardTableRS.hpp
===================================================================
--- memory/cardTableRS.hpp  (revision 86)
+++ memory/cardTableRS.hpp  (working copy)
@@ -101,6 +101,8 @@

   CardTableModRefBS* ct_bs() { return &_ct_bs; }

+  jbyte * base_map() { return &(_ct_bs._byte_map[0]); }
+
   // Override.
   void prepare_for_younger_refs_iterate(bool parallel);

                                    

Comments
SUGGESTED FIX

See second entry in description field.
                                     
2007-05-17
EVALUATION

See description; fix under internal test and review.
                                     
2007-05-17
SUGGESTED FIX

Event:            putback-to
Parent workspace: /net/jano.sfbay/export/disk05/hotspot/ws/main/gc_baseline
                  (jano.sfbay:/export/disk05/hotspot/ws/main/gc_baseline)
Child workspace:  /net/prt-web.sfbay/prt-workspaces/20070525143415.ysr.mustang/workspace
                  (prt-web:/net/prt-web.sfbay/prt-workspaces/20070525143415.ysr.mustang/workspace)
User:             ysr

Comment:

---------------------------------------------------------

Job ID:                 20070525143415.ysr.mustang
Original workspace:     karachi:/net/jano.sfbay/export/hotspot/users1/ysr/mustang
Submitter:              ysr
Archived data:          /net/prt-archiver.sfbay/data/archived_workspaces/main/gc_baseline/2007/20070525143415.ysr.mustang/
Webrev:                 http://prt-web.sfbay.sun.com/net/prt-archiver.sfbay/data/archived_workspaces/main/gc_baseline/2007/20070525143415.ysr.mustang/workspace/webrevs/webrev-2007.05.25/index.html

Fixed 6559052: [OpenJDK] CardTableRS::do_MemRegion() could attempt out-of-bounds translation in addr_for()

   webrev: http://analemma.SFBay.Sun.COM/net/jano/export/disk05/hotspot/users/ysr/mustang/webrev.6559052

This is a potential problem (but not a current one, in any current
HotSpot configurations), where a do_MemRegion() call on a MemRegion
that includes a least end (i.e. first page) of the Java heap (or
a first page of a "heap chunk" in the case of future "chunked heaps")
can result in an attempt to do an out-of-bounds addr_for() translation.

Our fix is to reorganize the code in the do_MemRegion() method
so as to delay the translation until we know that it will be safe.

Acknowledgements: This problem was pointed out by OpenJDK
community member Neo Jia who also provided an initial solution,
of which the above is a refinement; see bug id or openjdk's gc-dev
alias for more details.

Fix Verified: n (see "Testing" above; note that this is not an issue
 for current versions of HotSpot)
Verification Testing: (see "Testing" above; note that this is not an issue
 for current versions of HotSpot)

Other Testing: PRT, refworkload, runThese quick/testbase

Reviewed by: jmasa, neojia@openjdk, pbk

Files:
update: src/share/vm/memory/cardTableRS.cpp

Examined files: 3964

Contents Summary:
       1   update
    3963   no action (unchanged)
                                     
2007-05-30



Hardware and Software, Engineered to Work Together