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

Klass enqueue element size calculation wrong when traceid value cross compress limit

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: P2
    • Resolution: Fixed
    • Affects Version/s: 17, 18
    • Fix Version/s: 17
    • Component/s: hotspot
    • Labels:
    • Subcomponent:
      jfr
    • Resolved In Build:
      b28
    • Verification:
      Not verified

      Backports

        Description

        jvm!JfrArtifactCallbackHost<Klass const *,CompositeFunctor<Klass const *,JfrTypeWriterHost<JfrPredicatedTypeWriterImplHost<Klass const *,SerializePredicate<Klass const *>,&write__klass>,164>,KlassArtifactRegistrator> >::do_artifact:
        00007ffc`4b2aaa70 48895c2408 mov qword ptr [rsp+8],rbx
        00007ffc`4b2aaa75 4889542410 mov qword ptr [rsp+10h],rdx
        00007ffc`4b2aaa7a 57 push rdi
        00007ffc`4b2aaa7b 4883ec20 sub rsp,20h
        00007ffc`4b2aaa7f 488b7910 mov rdi,qword ptr [rcx+10h]
        00007ffc`4b2aaa83 488bda mov rbx,rdx
        00007ffc`4b2aaa86 488d542438 lea rdx,[rsp+38h] // <<---- stack position lea to rdx
        00007ffc`4b2aaa8b 488b0f mov rcx,qword ptr [rdi]
        00007ffc`4b2aaa8e e8ddf9ffff call jvm!JfrTypeWriterHost<JfrPredicatedTypeWriterImplHost<Klass const *,SerializePredicate<Klass const *>,&write__klass>,164>::operator() (00007ffc`4b2aa470)
        00007ffc`4b2aaa93 84c0 test al,al

        jvm!JfrTypeWriterHost<JfrPredicatedTypeWriterImplHost<Klass const *,SerializePredicate<Klass const *>,&write__klass>,164>::operator():
        00007ffc`4b2aa470 4053 push rbx
        00007ffc`4b2aa472 4883ec20 sub rsp,20h
        00007ffc`4b2aa476 80790800 cmp byte ptr [rcx+8],0
        00007ffc`4b2aa47a 488bd9 mov rbx,rcx
        00007ffc`4b2aa47d 488b12 mov rdx,qword ptr [rdx] // read stack position 000000ea`866ff648 -- was set via lea rdx,[rsp+38h] ; rdx == 00000000`40122ee2
        0007ffc`4b2aa480 7510 jne jvm!JfrTypeWriterHost<JfrPredicatedTypeWriterImplHost<Klass const *,SerializePredicate<Klass const *>,&write__klass>,164>::operator()+0x22 (00007ffc`4b2aa492)
        00007ffc`4b2aa482 8b82a8000000 mov eax,dword ptr [rdx+0A8h] // <<--- expects rdx to be an uncompressed klass, the attempt to read the _traceid field traps (offset 0xa8 is _traceid field offset for the klass, in product builds)

        dps @rsp
        000000ea`866ff5e0 000000ea`866ff800 // <<-- sub 0x20
        000000ea`866ff5e8 00000237`d88c9ea0
        000000ea`866ff5f0 00000000`00000009
        000000ea`866ff5f8 00000000`00000000
        000000ea`866ff600 00000000`40122ee2 // <<-- push rbx
        000000ea`866ff608 00007ffc`4b2aaa93 jvm!JfrArtifactCallbackHost,&write__klass>,164>,KlassArtifactRegistrator> >::do_artifact+0x23 [t:\workspace\open\src\hotspot\share\jfr\recorder\checkpoint\types\jfrTypeSetUtils.hpp @ 73]
        000000ea`866ff610 00000008`001d6988
        000000ea`866ff618 00000000`00000000
        000000ea`866ff620 00000000`00000000
        000000ea`866ff628 00000000`00000000
        000000ea`866ff630 00000000`40122ee2 // <<-- rdi pushed
        000000ea`866ff638 00007ffc`4b2a66bd jvm!JfrLinkedList::iterate::ElementDispatch > >+0x10d [t:\workspace\open\src\hotspot\share\jfr\utilities\jfrLinkedList.inline.hpp @ 83]
        000000ea`866ff640 00000237`d88c9df0 // <<-- rbx saved
        000000ea`866ff648 00000000`40122ee2 // <<-- rdx saved ; lea rdx [rsp+0x38] == 000000ea`866ff648
        000000ea`866ff650 00000000`00000000
        000000ea`866ff658 00000000`00000000


        // enqueued elements

        dps 00000237d88c9df0 - 0x40
        00000237`d88c9db0 010d02d8`000efd24 // <<-- compressed element
        00000237`d88c9db8 010d02d8`000efd24
        00000237`d88c9dc0 010d02d8`000efd24
        00000237`d88c9dc8 00050028`00000620
        00000237`d88c9dd0 00050028`00000620
        00000237`d88c9dd8 00050028`00000620
        00000237`d88c9de0 001d6988`00001690
        00000237`d88c9de8 001d6988`00001690
        00000237`d88c9df0 00000000`40122eca // <<--- next element pos in rbx == 00000237`d88c9df0 ; 00000000`40122eca & 2 == 2 => uncompressed entry -> uncompressed klass should be in next word / slot
        00000237`d88c9df8 00000000`40122ee2 // <<--- expected the uncompressed klass here, but due to element size error, the traceid for the next element has overwritten the uncompressed klass slot for the previous element
        00000237`d88c9e00 00000000`40122ea2
        00000237`d88c9e08 0000fa38`00000574
        00000237`d88c9e10 00c38080`000020e8
        00000237`d88c9e18 0017a218`00001100
        00000237`d88c9e20 00c0ec28`00001de0
        00000237`d88c9e28 00c0ec28`00001de0


        // the element size is a function of the traceid value, but in the EpochQueue form, element_size is declared "static" -> will not recalculate for uncompressed entries, i.e. compressed == 8 bytes, uncompressed == 16 bytes

        // jfrEpochQueue.inline.hpp

        template <template <typename> class ElementPolicy>
        void JfrEpochQueue<ElementPolicy>::enqueue(JfrEpochQueue<ElementPolicy>::TypePtr t) {
          assert(t != NULL, "invariant");
          static size_t element_size = _policy.element_size(t); // <<---- element_size "static"
          BufferPtr buffer = storage_for_element(t, element_size);
          assert(buffer != NULL, "invariant");
          _policy.store_element(t, buffer);
          buffer->set_pos(element_size); // <<----- sets next position to point at the uncompressed klass, which is then overwritten with the traceid for the next store
        }

        // In addition, this is not correct:

        // jfrTraceIdKlassQueue.cpp:

        static void store_element(const Klass* klass, u1* pos) {
          assert(pos != NULL, "invariant");
          assert(klass != NULL, "invariant");
          traceid id = JfrTraceId::load_raw(klass);
        #ifdef VM_LITTLE_ENDIAN
          id <<= METADATA_SHIFT;
        #endif
          if (can_compress_element(id)) { <<--- the id is evaluated for compression _after_ shift; this means that size calculations could have reported the size to accommodate a compressed entry (evaluated before shift), but as part of actually encoding the element, it could evaluate to an uncompressed element (evaluated after shift), hence overwriting allocated storage space.
            store_compressed_element(id, klass, pos);
          } else {
            store_uncompressed_element(id, klass, pos);
          }
        }

        // To reach this problem the system would have needed to load ~256 million classes.

          Attachments

            Issue Links

              Activity

                People

                Assignee:
                mgronlun Markus Grönlund
                Reporter:
                mgronlun Markus Grönlund
                Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                  Dates

                  Created:
                  Updated:
                  Resolved: