Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-6559052

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

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: P5
    • Resolution: Fixed
    • Affects Version/s: 7
    • Fix Version/s: hs10
    • Component/s: hotspot
    • Labels:
    • Subcomponent:
      gc
    • Introduced In Build:
      1.4.1
    • Introduced In Version:
    • Resolved In Build:
      b14
    • CPU:
      generic
    • OS:
      generic

      Backports

        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);

          Attachments

            Issue Links

              Activity

                People

                • Assignee:
                  ysr Y. Ramakrishna
                  Reporter:
                  ysr Y. Ramakrishna
                • Votes:
                  0 Vote for this issue
                  Watchers:
                  1 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Imported:
                    Indexed: