RFR: 8289688: jfr command hangs when it processes invalid file [v2]

[Yasumasa Suenaga]

On Mon, 25 Jul 2022 13:30:03 GMT, Erik Gahlin <egahlin at openjdk.org> wrote:

>> `ChunkHeader::refresh` need to satisfy to exit loop following conditions:
>> 
>> * File state is "stable" (get same state twice (L123 and L131))
>> * `metadataPosition` is not (greater than) 0
>> 
>> `fileState1` comes from `readFileState()` which uses `pollWait()` as you mentioned, so we can deem the recording data is "stable" if L134 is true.
>> 
>> 
>> 123             byte fileState1 = readFileState();
>>  :
>> 131             byte fileState2 =  input.readPhysicalByte();
>> 132             input.positionPhysical(absoluteChunkStart + FLAG_BYTE_POSITION);
>> 133             int flagByte = input.readPhysicalByte();
>> 134             if (fileState1 == fileState2) { // valid header
>> 135                 finished = fileState1 == 0;
>> 136                 if (metadataPosition != 0) {
>>  :
>> 164                     return;
>> 165                 }
>> 
>> 
>> In addition, we can deem that recording data would not be updated if `finished` is set to `true` in L135. Thus I think we can fix this problem with following patch. How about this?
>> 
>> 
>> diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/consumer/ChunkHeader.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/consumer/ChunkHeader.java
>> index 8c8ec55f0da..8648c030f06 100644
>> --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/consumer/ChunkHeader.java
>> +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/consumer/ChunkHeader.java
>> @@ -162,6 +162,8 @@ public final class ChunkHeader {
>>                      Logger.log(LogTag.JFR_SYSTEM_PARSER, LogLevel.INFO, "Chunk: finalChunk=" + finalChunk);
>>                      absoluteChunkEnd = absoluteChunkStart + chunkSize;
>>                      return;
>> +                } else if (finished) {
>> +                    throw new IOException("metadataPosition is 0 but chunk header is valid.");
>>                  }
>>              }
>>          }
>> 
>> 
>> I understand that producer (Native Image in this case) should be fixed not to out invalid recording file. However we need to notify errors when the user passes invalid file to jfr command. So I've proposed this to jdk project.
>
>> diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/consumer/ChunkHeader.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/consumer/ChunkHeader.java
>> index 8c8ec55f0da..8648c030f06 100644
>> --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/consumer/ChunkHeader.java
>> +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/consumer/ChunkHeader.java
>> @@ -162,6 +162,8 @@ public final class ChunkHeader {
>>                      Logger.log(LogTag.JFR_SYSTEM_PARSER, LogLevel.INFO, "Chunk: finalChunk=" + finalChunk);
>>                      absoluteChunkEnd = absoluteChunkStart + chunkSize;
>>                      return;
>> +                } else if (finished) {
>> +                    throw new IOException("metadataPosition is 0 but chunk header is valid.");
>>                  }
>>              }
>>          }
>> ```
>> 
> 
> This may work, values are read reliably and when a chunk is finished, it should always have metadata section, or something is wrong with the file. 
> 
> That said, when the file format was designed the metadata pointer in the header was meant as a hint (not a mandatory thing) to prevent a parser for reading the file twice. Neither JMC or JDK parser supports reading recordings without the metadata pointer hint AFAIK, so that ship may have sailed. If the use case should arrive, the ChunkHeader class must be modified anyway (and the above check replaced with an iteration over all the events to find the offset). 
> 
> Have you looked at the HotSpot implementation to make sure fileState is never 0, unless the metadata pointer hint has been written? If I remember correctly, the initial value is 1, but I haven't checked. I looked at the RemoteRecordingStream class and it looks like it can't happen there, if the output from the JVM is OK.
> 
> I think the message could be "No metadata event found in finished chunk".

Thanks @egahlin for your comment! I updated this PR with your suggestion.

> Have you looked at the HotSpot implementation to make sure fileState is never 0, unless the metadata pointer hint has been written?

In JfrChunkWriter.cpp, the file state is always set to `0` when a chunk is finalized.

```c++
101   void write_generation(bool finalize) {
102     _writer->be_write(finalize ? COMPLETE : _chunk->generation());
103     _writer->be_write(PAD);
104   }
105
106   void write_next_generation(bool finalize) {
107     _writer->be_write(finalize ? COMPLETE : _chunk->next_generation());
108     _writer->be_write(PAD);
109   }

-------------

PR: https://git.openjdk.org/jdk/pull/9363