RFR (M): 8129419: heapDumper.cpp: assert(length_in_bytes > 0) failed: nothing to copy

[Andreas Eriksson]

Thanks!

On 2016-01-18 14:10, Dmitry Samersoff wrote:
> Andreas,
>
> Thumbs up! (Reviewed)
>
> -Dmitry
>
> On 2016-01-18 16:07, Andreas Eriksson wrote:
>>
>> On 2016-01-18 13:58, Dmitry Samersoff wrote:
>>> Andreas,
>>>
>>> On 2016-01-18 15:26, Andreas Eriksson wrote:
>>>> I believe I then have to cast it to size_t at 496, 497 and 498?
>>> I hope not.
>>>
>>>> I'm not sure how write works in regards to returning ssize_t vs a size_t
>>>> length.
>>> It's a tricky part.
>>>
>>> The maximum number of bytes that could be actually written by write() in
>>> one shot is implementation dependent and limited by fs (or VFS) driver.
>>> Typically it is less than MAX_INT.
>>>
>>> If len overflow it, write just return the number of bytes actually
>>> written without setting an error.
>>>
>>>> Maybe I should change it back to checking for return value < 0 (instead
>>>> of only checking for OS_ERR), and then keep 'n' as ssize_t?
>>> I would prefer this way.
>> Done.
>> Uploaded new webrev if you want to take a look:
>> http://cr.openjdk.java.net/~aeriksso/8129419/webrev.03/hotspot/
>>
>> - Andreas
>>
>>> -Dmitry
>>>
>>>> - Andreas
>>>>
>>>> On 2016-01-18 13:11, Dmitry Samersoff wrote:
>>>>> Андреас,
>>>>>
>>>>> 481: It's better to declare n as ssize_t and remove cast at 488
>>>>>
>>>>> Looks good for me!
>>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 2016-01-18 14:42, Andreas Eriksson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> New webrev:
>>>>>> http://cr.openjdk.java.net/~aeriksso/8129419/webrev.02/hotspot/
>>>>>>
>>>>>> Write_internal handles writes less than len, and EINTR.
>>>>>>
>>>>>> EINTR is taken care of by os::write, but to use it I had to remove an
>>>>>> assert from the solaris os::write, which checked that a JavaThread was
>>>>>> in_native. Since heap dumping is done from the vm_thread (not a
>>>>>> JavaThread) the assert crashed the VM.
>>>>>>
>>>>>> Regards,
>>>>>> Andreas
>>>>>>
>>>>>> On 2016-01-13 13:39, Andreas Eriksson wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 2015-12-29 21:27, Dmitry Samersoff wrote:
>>>>>>>> Andreas,
>>>>>>>>
>>>>>>>> Great work. All but write_internal looks good for me (see below).
>>>>>>>>
>>>>>>>>
>>>>>>>> HprofReader.java:
>>>>>>>>
>>>>>>>> Nit -  length -= skipped; should be after if skipped == 0
>>>>>>>>
>>>>>>>> heapDumper.cpp:480
>>>>>>>>
>>>>>>>> 1. For windows you can do :
>>>>>>>>         assert(len < (size_t)UINT_MAX, ... );
>>>>>>>>         ::write( ..., (uint) (len & UINT_MAX));
>>>>>>>>
>>>>>>>> 2. You should check for n < len and if it is true,
>>>>>>>> deal with errno. n = 0 is legitimate case,
>>>>>>>> so assert is not necessary.
>>>>>>> I only think n = 0 is valid if write is called with length 0, which
>>>>>>> write_internal will never do.
>>>>>>> Otherwise, according to posix, n will be -1 if an error occurred, or
>>>>>>> greater than zero if some bytes were written. [1]
>>>>>>> If some bytes but not all were written then the while(len > 0) loop
>>>>>>> will make sure we try to write the rest of the bytes.
>>>>>>> It looks like windows write should never return 0 when len != 0
>>>>>>> either. [2]
>>>>>>>
>>>>>>> I should however handle the -1 result with errno EINTR, working on a
>>>>>>> new webrev.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Andreas
>>>>>>>
>>>>>>> [1]
>>>>>>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html
>>>>>>> If write() is interrupted by a signal before it writes any data, it
>>>>>>> shall return -1 with errno set to [EINTR].
>>>>>>> If write() is interrupted by a signal after it successfully writes
>>>>>>> some data, it shall return the number of bytes written.
>>>>>>>
>>>>>>> [2] https://msdn.microsoft.com/en-us/library/1570wh78(v=vs.80).aspx
>>>>>>>
>>>>>>>> -Dmitry
>>>>>>>>
>>>>>>>> On 2015-12-28 17:02, Andreas Eriksson wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Here is the webrev for type changes.
>>>>>>>>> Top-repo:
>>>>>>>>> http://cr.openjdk.java.net/~aeriksso/8129419/webrev.01/jdk9-hs-rt/
>>>>>>>>> Hotspot:
>>>>>>>>> http://cr.openjdk.java.net/~aeriksso/8129419/webrev.01/hotspot/
>>>>>>>>>
>>>>>>>>> The windows_x64 native write function uses a length of type
>>>>>>>>> uint, not
>>>>>>>>> size_t. I added an ifdef for that case and handled it, but better
>>>>>>>>> suggestions are welcome.
>>>>>>>>>
>>>>>>>>> Also found and fixed another problem in the hprof parser when
>>>>>>>>> skipping
>>>>>>>>> over bytes.
>>>>>>>>>
>>>>>>>>> I've not included the tests, they have OOM issues on several JPRT
>>>>>>>>> hosts,
>>>>>>>>> and until I've figured out a way to fix that I wont be pushing
>>>>>>>>> them.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Andreas
>>>>>>>>>
>>>>>>>>> On 2015-12-14 19:34, Andreas Eriksson wrote:
>>>>>>>>>> Hi Dmitry, thanks for looking at this.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2015-12-14 18:30, Dmitry Samersoff wrote:
>>>>>>>>>>> Andreas,
>>>>>>>>>>>
>>>>>>>>>>> Nice cleanup.
>>>>>>>>>>>
>>>>>>>>>>> Some generic comments below.
>>>>>>>>>>>
>>>>>>>>>>> 1. It would be excellent if you can split webrev into two parts,
>>>>>>>>>>> one
>>>>>>>>>>> part with types cleanup and other part with array truncation
>>>>>>>>>>> related
>>>>>>>>>>> changes or ever push these changes separately as it address
>>>>>>>>>>> different
>>>>>>>>>>> problems.
>>>>>>>>>>>
>>>>>>>>>>> Type cleanup could be reviewed quickly, but review of array
>>>>>>>>>>> truncation
>>>>>>>>>>> requires some time.
>>>>>>>>>>>
>>>>>>>>>>> (We already have two different CRs: JDK-8129419 for type cleanup
>>>>>>>>>>> and
>>>>>>>>>>> JDK-8144732 for array truncation)
>>>>>>>>>> Yes, that's a good point.
>>>>>>>>>>
>>>>>>>>>>> 2.
>>>>>>>>>>> it might be better to use size_t and jlong (or julong) across all
>>>>>>>>>>> file
>>>>>>>>>>> and get rid of all other types with a few exclusion.
>>>>>>>>>> I'll see what I can do, and we can look at it closer once I've
>>>>>>>>>> split
>>>>>>>>>> the type changes into a separate webrev.
>>>>>>>>>>
>>>>>>>>>>> 3.
>>>>>>>>>>> Each assert costs some time in nightly testing, so we probably
>>>>>>>>>>> don't
>>>>>>>>>>> need as much asserts.
>>>>>>>>>> Alright.
>>>>>>>>>>
>>>>>>>>>>> 4. write_internal:
>>>>>>>>>>>
>>>>>>>>>>>        There are couple of cases where ::write writes less
>>>>>>>>>>> bytes than
>>>>>>>>>>> requested and doesn't return -1.
>>>>>>>>>>>        So we should check if ::write returns a value less that len
>>>>>>>>>>> and if it
>>>>>>>>>>> happens deal with errno - either repeat entire write
>>>>>>>>>>> attempt(EINTR/EAGAIN) or abort writing.
>>>>>>>>>> Actually, I meant to ask about that, but forgot:
>>>>>>>>>> I tried using os::write, which handles EINTR/EAGAIN if necessary
>>>>>>>>>> (depending on platform), but Solaris has an assert that fails:
>>>>>>>>>> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/file/5a42c1dde332/src/os/solaris/vm/os_solaris.cpp#l5692
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> It checks that os::write is called by a JavaThread that is
>>>>>>>>>> in_native,
>>>>>>>>>> which we are not, since we are in the VM_thread doing a
>>>>>>>>>> safepoint_op.
>>>>>>>>>> Does anyone know if that assert is really needed? It's only
>>>>>>>>>> done for
>>>>>>>>>> Solaris.
>>>>>>>>>>
>>>>>>>>>>> 5. we should handle zero-length array in
>>>>>>>>>>> calculate_array_max_length -
>>>>>>>>>>> it's a legitimate case
>>>>>>>>>> Not sure what you mean here, I believe it does handle zero-length
>>>>>>>>>> arrays.
>>>>>>>>>> It should just fall through without taking any of the if-clauses.
>>>>>>>>>> (Or do you mean that I should add a return immediately at the
>>>>>>>>>> top if
>>>>>>>>>> length is zero?)
>>>>>>>>>>
>>>>>>>>>>> 6. max_juint is less than SegmentedHeapDumpThreshold so
>>>>>>>>>>> non-segmented
>>>>>>>>>>> heapdump can't contain huge array and it's better to check for
>>>>>>>>>>> segmented
>>>>>>>>>>> dump before any other checks.
>>>>>>>>>> Correct me if I'm wrong here, but SegmentedHeapDumpThreshold
>>>>>>>>>> could in
>>>>>>>>>> theory be set so that we have a non-segmented heap dump while
>>>>>>>>>> still
>>>>>>>>>> having huge arrays.
>>>>>>>>>> Not sure if this is worth taking into account, since it most
>>>>>>>>>> likely
>>>>>>>>>> would lead to other problems as well, and the flag is develop
>>>>>>>>>> only, so
>>>>>>>>>> it can't happen in product builds.
>>>>>>>>>> What do you think would be best to do here?
>>>>>>>>>>
>>>>>>>>>> - Andreas
>>>>>>>>>>
>>>>>>>>>>> -Dmitry
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2015-12-14 18:38, Andreas Eriksson wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> Please review this fix for dumping of long arrays, and general
>>>>>>>>>>>> cleanup
>>>>>>>>>>>> of types in heapDumper.cpp.
>>>>>>>>>>>>
>>>>>>>>>>>> Problem:
>>>>>>>>>>>> At several places in heapDumper.cpp overflows could happen when
>>>>>>>>>>>> dumping
>>>>>>>>>>>> long arrays.
>>>>>>>>>>>> Also the hprof format uses an u4 as a record length field, but
>>>>>>>>>>>> arrays
>>>>>>>>>>>> can be longer than that (counted in bytes).
>>>>>>>>>>>>
>>>>>>>>>>>> Fix:
>>>>>>>>>>>> Many types that were previously signed are changed to equivalent
>>>>>>>>>>>> unsigned types and/or to a larger type to prevent overflow.
>>>>>>>>>>>> The bulk of the change though is the addition of
>>>>>>>>>>>> calculate_array_max_length, which for a given array returns the
>>>>>>>>>>>> number
>>>>>>>>>>>> of elements we can dump. That length is then used to truncate
>>>>>>>>>>>> arrays
>>>>>>>>>>>> that are too long.
>>>>>>>>>>>> Whenever an array is truncated a warning is printed:
>>>>>>>>>>>> Java HotSpot(TM) 64-Bit Server VM warning: cannot dump array of
>>>>>>>>>>>> type
>>>>>>>>>>>> object[] with length 1,073,741,823; truncating to length
>>>>>>>>>>>> 536,870,908
>>>>>>>>>>>>
>>>>>>>>>>>> Much of the rest of the change is moving functions needed by
>>>>>>>>>>>> calculate_array_max_length to the DumpWriter or DumperSupport
>>>>>>>>>>>> class so
>>>>>>>>>>>> that they can be accessed.
>>>>>>>>>>>>
>>>>>>>>>>>> Added a test that relies on the hprof parser, which also had a
>>>>>>>>>>>> couple of
>>>>>>>>>>>> overflow problems (top repo changes).
>>>>>>>>>>>> I've also run this change against a couple of other tests, but
>>>>>>>>>>>> they are
>>>>>>>>>>>> problematic in JPRT because they are using large heaps and
>>>>>>>>>>>> lots of
>>>>>>>>>>>> disk.
>>>>>>>>>>>>
>>>>>>>>>>>> Bug:
>>>>>>>>>>>> 8129419: heapDumper.cpp: assert(length_in_bytes > 0) failed:
>>>>>>>>>>>> nothing to
>>>>>>>>>>>> copy
>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8129419
>>>>>>>>>>>>
>>>>>>>>>>>> Also fixed in this change is the problems seen in these two
>>>>>>>>>>>> bugs:
>>>>>>>>>>>> 8133317: Integer overflow in heapDumper.cpp leads to corrupt
>>>>>>>>>>>> HPROF
>>>>>>>>>>>> dumps
>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8133317
>>>>>>>>>>>>
>>>>>>>>>>>> 8144732: VM_HeapDumper hits assert with bad dump_len
>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8144732
>>>>>>>>>>>>
>>>>>>>>>>>> Webrev:
>>>>>>>>>>>> Top repo:
>>>>>>>>>>>> http://cr.openjdk.java.net/~aeriksso/8129419/webrev.00/jdk9-hs-rt/
>>>>>>>>>>>>
>>>>>>>>>>>> Hotspot:
>>>>>>>>>>>> http://cr.openjdk.java.net/~aeriksso/8129419/webrev.00/hotspot/
>>>>>>>>>>>>
>>>>>>>>>>>> Regards,
>>>>>>>>>>>> Andreas
>