[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR (M): 8129419: heapDumper.cpp: assert(length_in_bytes > 0) failed: nothing to copy
From:       Dmitry Samersoff <dmitry.samersoff () oracle ! com>
Date:       2015-12-29 20:27:52
Message-ID: 5682ECC8.2040309 () oracle ! com
[Download RAW message or body]

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.

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


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
[prev in list] [next in list] [prev in thread] [next in thread]