JDK-6559052 : [OpenJDK] CardTableRS::do_MemRegion() could attempt out-of-bounds translation in addr_for()
  • Type: Bug
  • Component: hotspot
  • Sub-Component: gc
  • Affected Version: 7
  • Priority: P5
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2007-05-17
  • Updated: 2010-04-02
  • Resolved: 2007-06-20
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 6 JDK 7 Other
6u4Fixed 7Fixed hs10Fixed
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 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)
30-05-2007

SUGGESTED FIX See second entry in description field.
17-05-2007

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