RFR: 8297402: jfr tool processes large padded integers incorrectly
Could I have a review of a PR that fixes jfr tool reading of large padded integers. Some integer values are written to jfr recordings using write_padded_at_offset<u4>() method. Their size in recording is always 4 bytes. jfr tool reads all integer values by readInt() method, which doesn't take size into account. For padded values greater than (1 << 28), jfr tool tries to read 5 bytes from recording and fails. I suggest to fix reading of padded integers in jfr tool. If a value is written by write_padded_at_offset<u4>() then it should be read by readPaddedInt() method which doesn't read more than 4 bytes. (It can be less then 4 bytes due to JDK-8246260) Tested with jdk/jfr and tier1. jfr recording generated by TestHugeStackTracePool.java with jdk11 (attached to JDK-8297402) is successfully processed by jfr tool after the fix. ------------- Commit messages: - 8297402: jfr tool processes large padded integers incorrectly Changes: https://git.openjdk.org/jdk/pull/11289/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11289&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8297402 Stats: 27 lines in 5 files changed: 20 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/11289.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11289/head:pull/11289 PR: https://git.openjdk.org/jdk/pull/11289
On Tue, 22 Nov 2022 15:06:20 GMT, Ekaterina Vergizova <evergizova@openjdk.org> wrote:
Could I have a review of a PR that fixes jfr tool reading of large padded integers.
Some integer values are written to jfr recordings using write_padded_at_offset<u4>() method. Their size in recording is always 4 bytes. jfr tool reads all integer values by readInt() method, which doesn't take size into account. For padded values greater than (1 << 28), jfr tool tries to read 5 bytes from recording and fails.
I suggest to fix reading of padded integers in jfr tool. If a value is written by write_padded_at_offset<u4>() then it should be read by readPaddedInt() method which doesn't read more than 4 bytes. (It can be less then 4 bytes due to JDK-8246260)
Tested with jdk/jfr and tier1. jfr recording generated by TestHugeStackTracePool.java with jdk11 (attached to JDK-8297402) is successfully processed by jfr tool after the fix.
Maybe I am missing something, but the reason padded integer are used is because the size is not known until all the data has been written for an event, so the JVM/JDK reserves 4 bytes up front and if not all bytes are used, leftmost bytes are padded with 128, which corresponds to 0. If data can't fit in 2 ^28, maybe the JVM should split the data into two events? The JVM should not write data that indicates a fifth byte can be read. I think this is where the bug is. Markus has done some work in the area. ------------- PR: https://git.openjdk.org/jdk/pull/11289
On Tue, 22 Nov 2022 15:06:20 GMT, Ekaterina Vergizova <evergizova@openjdk.org> wrote:
Could I have a review of a PR that fixes jfr tool reading of large padded integers.
Some integer values are written to jfr recordings using write_padded_at_offset<u4>() method. Their size in recording is always 4 bytes. jfr tool reads all integer values by readInt() method, which doesn't take size into account. For padded values greater than (1 << 28), jfr tool tries to read 5 bytes from recording and fails.
I suggest to fix reading of padded integers in jfr tool. If a value is written by write_padded_at_offset<u4>() then it should be read by readPaddedInt() method which doesn't read more than 4 bytes. (It can be less then 4 bytes due to JDK-8246260)
Tested with jdk/jfr and tier1. jfr recording generated by TestHugeStackTracePool.java with jdk11 (attached to JDK-8297402) is successfully processed by jfr tool after the fix.
Currently, the JVM does not check 2 ^ 28 limit in most cases when writing padded integers. For example, it doesn't validate or reject values for a large pool of stacktraces. If such values are actually invalid, then I'll try to prepare a fix that splits the data into two events, if possible. ------------- PR: https://git.openjdk.org/jdk/pull/11289
On Tue, 22 Nov 2022 18:31:34 GMT, Ekaterina Vergizova <evergizova@openjdk.org> wrote:
Could I have a review of a PR that fixes jfr tool reading of large padded integers.
Some integer values are written to jfr recordings using write_padded_at_offset<u4>() method. Their size in recording is always 4 bytes. jfr tool reads all integer values by readInt() method, which doesn't take size into account. For padded values greater than (1 << 28), jfr tool tries to read 5 bytes from recording and fails.
I suggest to fix reading of padded integers in jfr tool. If a value is written by write_padded_at_offset<u4>() then it should be read by readPaddedInt() method which doesn't read more than 4 bytes. (It can be less then 4 bytes due to JDK-8246260)
Tested with jdk/jfr and tier1. jfr recording generated by TestHugeStackTracePool.java with jdk11 (attached to JDK-8297402) is successfully processed by jfr tool after the fix.
Currently, the JVM does not check 2 ^ 28 limit in most cases when writing padded integers. For example, it doesn't validate or reject values for a large pool of stacktraces.
If such values are actually invalid, then I'll try to prepare a fix that splits the data into two events, if possible.
Hi @kvergizova, thanks for noticing this. I ran into this situation a couple of months back and have an idea and suggestion to handle this situation. If you don't feel like digging too deep, I can either help you or take over this entirely. Best regards Markus ------------- PR: https://git.openjdk.org/jdk/pull/11289
On Wed, 23 Nov 2022 21:48:16 GMT, Markus Grönlund <mgronlun@openjdk.org> wrote:
Currently, the JVM does not check 2 ^ 28 limit in most cases when writing padded integers. For example, it doesn't validate or reject values for a large pool of stacktraces.
If such values are actually invalid, then I'll try to prepare a fix that splits the data into two events, if possible.
Hi @kvergizova, thanks for noticing this.
I ran into this situation a couple of months back and have an idea and suggestion to handle this situation. If you don't feel like digging too deep, I can either help you or take over this entirely.
Best regards Markus
Hi @mgronlun, thanks for the respond, I really appreciate your help. I have two different ideas how to handle this issue: - update WriteContent, Content and their derivatives to write a subset of elements in process() methods and check for 2^28 size overflow during writing. If an overflow occurs, duplicate the event header and process the rest of the elements. - reserve 5 bytes instead of 4 for some padded integers that are potentially prone to overflow if LEB128 encoder is used (e.g. checkpoint event size for stack traces or string pool). This seems to be pretty easy to implement, but will result in a slight increase in jfr recordings size. What do you think about it and what is your suggestion? If you prefer to take over this issue entirely and plan to do so in the near future, that would also be great. ------------- PR: https://git.openjdk.org/jdk/pull/11289
On Mon, 28 Nov 2022 16:41:34 GMT, Ekaterina Vergizova <evergizova@openjdk.org> wrote:
Hi @kvergizova, thanks for noticing this.
I ran into this situation a couple of months back and have an idea and suggestion to handle this situation. If you don't feel like digging too deep, I can either help you or take over this entirely.
Best regards Markus
Hi @mgronlun, thanks for the respond, I really appreciate your help.
I have two different ideas how to handle this issue:
- update WriteContent, Content and their derivatives to write a subset of elements in process() methods and check for 2^28 size overflow during writing. If an overflow occurs, duplicate the event header and process the rest of the elements. - reserve 5 bytes instead of 4 for some padded integers that are potentially prone to overflow if LEB128 encoder is used (e.g. checkpoint event size for stack traces or string pool). This seems to be pretty easy to implement, but will result in a slight increase in jfr recordings size.
What do you think about it and what is your suggestion? If you prefer to take over this issue entirely and plan to do so in the near future, that would also be great.
Hi @kvergizova, there is a way to handle this situation, specifically for checkpoint events. Since the raw information for these events are stored in-memory, in a queue before serializing to disk, it is possible to know how much space will be needed and make a reservation as a function of that. I have a patch in progress, so I can take over this issue. Thanks Markus ------------- PR: https://git.openjdk.org/jdk/pull/11289
On Tue, 22 Nov 2022 15:06:20 GMT, Ekaterina Vergizova <evergizova@openjdk.org> wrote:
Could I have a review of a PR that fixes jfr tool reading of large padded integers.
Some integer values are written to jfr recordings using write_padded_at_offset<u4>() method. Their size in recording is always 4 bytes. jfr tool reads all integer values by readInt() method, which doesn't take size into account. For padded values greater than (1 << 28), jfr tool tries to read 5 bytes from recording and fails.
I suggest to fix reading of padded integers in jfr tool. If a value is written by write_padded_at_offset<u4>() then it should be read by readPaddedInt() method which doesn't read more than 4 bytes. (It can be less then 4 bytes due to JDK-8246260)
Tested with jdk/jfr and tier1. jfr recording generated by TestHugeStackTracePool.java with jdk11 (attached to JDK-8297402) is successfully processed by jfr tool after the fix.
This pull request has been closed without being integrated. ------------- PR: https://git.openjdk.org/jdk/pull/11289
participants (3)
-
Ekaterina Vergizova
-
Erik Gahlin
-
Markus Grönlund