ORC-1528: Fix readBytes potential overflow in RecordReaderUtils.ChunkReader#create #1662

yebukong · 2023-11-24T12:35:07Z

What changes were proposed in this pull request?

I have adjusted the calculation of reqBytes and readBytes ranges in the org.apache.orc.impl.RecordReaderUtils.ChunkReader#create method from Integer to Long, and added validation for Integer overflow
Adds more test cases

Why are the changes needed?

This PR aims to fix ORC-1528

How was this patch tested?

org.apache.orc.impl.TestRecordReaderUtils#testBufferChunkOffsetExceedsMaxInt

guiyanakuang · 2023-11-24T14:09:07Z

Thank you for making a PR, @yebukong

paliwalashish · 2023-11-24T16:50:13Z

java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java

@@ -770,18 +770,18 @@ void readRanges(FSDataInputStream file, boolean allocateDirect, double extraByte
    }

    static ChunkReader create(BufferChunk from, BufferChunk to) {
-      long f = Integer.MAX_VALUE;
-      long e = Integer.MIN_VALUE;
+      long f = Long.MAX_VALUE;


Recommendation: This is already existing so please feel free to ignore.
Since we are touching the code, Can we rename the variables for easy reading?
Couldn't understand the meaning of f, e, cf, ce 😢

Great, let me try.

Actually, I'm opposite to @mystic-lama 's comment.

We don't want to mix a bug fix and a style fix (or refactorying) in the same PR. It's because it increases the review complexity indeed.

If we want to rename this, it should be done before or after this PR.

Please don't change the existing variable name in this PR.

Ack, @dongjoon-hyun has a point. @yebukong we can do that in follow up PR or may be leave it as is :)
Apologies for the noise.

java/core/src/test/org/apache/orc/impl/TestRecordReaderUtils.java

dongjoon-hyun · 2023-11-25T04:34:51Z

java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java

@@ -770,18 +770,18 @@ void readRanges(FSDataInputStream file, boolean allocateDirect, double extraByte
    }

    static ChunkReader create(BufferChunk from, BufferChunk to) {
-      long f = Integer.MAX_VALUE;
-      long e = Integer.MIN_VALUE;
+      long f = Long.MAX_VALUE;


Actually, I'm opposite to @mystic-lama 's comment.

We don't want to mix a bug fix and a style fix (or refactorying) in the same PR. It's because it increases the review complexity indeed.

If we want to rename this, it should be done before or after this PR.

Please don't change the existing variable name in this PR.

dongjoon-hyun · 2023-11-25T04:36:34Z

Thank you for reporting a bug and making a PR, @yebukong .
Please make a renaming PR separately. I can merge that renaming PR first before this main PR.

guiyanakuang · 2023-11-25T04:48:20Z

java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java

-      reqBytes += ef - cf;
-      return new ChunkReader(from, to, (int) (e - f), reqBytes);
+      reqBytes += currentEnd - currentStart;
+      if (reqBytes >= Integer.MAX_VALUE) {


orc/java/core/src/java/org/apache/orc/impl/StringHashTableDictionary.java

Lines 61 to 65 in 22a06b4

/**

* The maximum size of array to allocate, value being the same as {@link java.util.Hashtable},

* given the fact that the stripe size could be increased to larger value by configuring "orc.stripe.size".

*/

private static final int MAX_ARRAY_SIZE = Integer.MAX_VALUE - 8;

I think it might be more accurate to extract and reuse this MAX_ARRAY_SIZE constant, the java maximum array length is not actually Integer.MAX_VALUE.

Should I define a MAX_ARRAY_SIZE constant in RecordReaderUtils ? I noticed it is marked as private in StringHashTableDictionary.

I tend to extract it into a utility class (e.g. IOUtils) and reuse the same constant.

I think we should remove MAX_ARRAY_SIZE from StringHashTableDictionary.java in this pr, the same concept should be kept in a single source (IOUtils).

I agree "the same concept should be kept in a single source".

Should we include the changes to remove the MAX_ARRAY_SIZE definition from StringHashTableDictionary.java in this PR?

@guiyanakuang Considering that this PR has been closed, is it still necessary to continue the discussion on the issues mentioned above?

yebukong · 2023-11-25T05:36:40Z

I've already maked a renaming PR, #1664. @dongjoon-hyun PTAL.
After the completion of renaming PR, i will push the changes to the main PR.

dongjoon-hyun · 2023-11-27T01:24:45Z

I've already maked a renaming PR, #1664. @dongjoon-hyun PTAL. After the completion of renaming PR, i will push the changes to the main PR.

To @yebukong , I repled on your new PR about why we cannot proceed that renaming PR first.

ORC-1530: Rename variables in RecordReaderUtils.ChunkReader#create #1664 (comment)

dongjoon-hyun · 2023-11-27T01:26:03Z

In short,

ORC-1528 should land at main/branch-1.9/branch-1.8 branches as a bug fix.
ORC-1530 should land at only main branch as an improvement or task PR

This reverts commit 88aecc6.

yebukong · 2023-11-27T07:02:22Z

I have pushed the latest code modifications. @dongjoon-hyun @guiyanakuang @mystic-lama PTAL.

java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java

paliwalashish · 2023-11-27T16:43:33Z

java/core/src/java/org/apache/orc/impl/IOUtils.java

@@ -30,6 +30,11 @@ public final class IOUtils {

  public static final int DEFAULT_BUFFER_SIZE = 8192;

+  /**
+   * The maximum size of array to allocate, value being the same as {@link java.util.Hashtable}


Can we add info like max array to allocate for what purpose? Please only add if the specific context is known, else we leave it as is.

https://github.com/openjdk/jdk/blob/jdk-17%2B35/src/java.base/share/classes/java/util/Hashtable.java#L391-L397

Because the comment for java.util.Hashtable#MAX_ARRAY_SIZE is detailed, I did not add excessive description.

paliwalashish

+1 Changes look good to me.

dongjoon-hyun

+1, LGTM.

…Reader#create - I have adjusted the calculation of `reqBytes` and `readBytes` ranges in the `org.apache.orc.impl.RecordReaderUtils.ChunkReader#create` method from `Integer` to `Long`, and added validation for `Integer` overflow - Adds more test cases This PR aims to fix [ORC-1528](https://issues.apache.org/jira/browse/ORC-1528) org.apache.orc.impl.TestRecordReaderUtils#testBufferChunkOffsetExceedsMaxInt Closes #1662 from yebukong/main. Lead-authored-by: yebukong <[email protected]> Co-authored-by: yebukong <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit e554765) Signed-off-by: Dongjoon Hyun <[email protected]>

dongjoon-hyun · 2023-11-27T20:54:46Z

Merged to main/1.9/1.8.

dongjoon-hyun · 2023-11-27T20:55:57Z

Welcome to the Apache ORC community, @yebukong .
I added you to the Apache ORC contributor group and assigned ORC-1528 to you.
Thank you again.

### What changes were proposed in this pull request? rename variables in RecordReaderUtils.ChunkReader#create ### Why are the changes needed? for easy reading see [PR 1662](#1662 (comment)) ### How was this patch tested? Closes #1664 from yebukong/ORC-1528_rename_variables. Authored-by: yebukong <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

yebukong · 2023-11-29T11:56:21Z

You're welcome, it's my pleasure. @dongjoon-hyun

…Reader#create ### What changes were proposed in this pull request? - I have adjusted the calculation of `reqBytes` and `readBytes` ranges in the `org.apache.orc.impl.RecordReaderUtils.ChunkReader#create` method from `Integer` to `Long`, and added validation for `Integer` overflow - Adds more test cases ### Why are the changes needed? This PR aims to fix [ORC-1528](https://issues.apache.org/jira/browse/ORC-1528) ### How was this patch tested? org.apache.orc.impl.TestRecordReaderUtils#testBufferChunkOffsetExceedsMaxInt Closes apache#1662 from yebukong/main. Lead-authored-by: yebukong <[email protected]> Co-authored-by: yebukong <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

### What changes were proposed in this pull request? rename variables in RecordReaderUtils.ChunkReader#create ### Why are the changes needed? for easy reading see [PR 1662](apache#1662 (comment)) ### How was this patch tested? Closes apache#1664 from yebukong/ORC-1528_rename_variables. Authored-by: yebukong <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

Fix readBytes potential overflow in RecordReaderUtils#create

ffc6b16

github-actions bot added the JAVA label Nov 24, 2023

yebukong changed the title ~~[ORC-1528]Fix readBytes potential overflow in RecordReaderUtils#create~~ ORC-1528: Fix readBytes potential overflow in RecordReaderUtils#create Nov 24, 2023

paliwalashish reviewed Nov 24, 2023

View reviewed changes

rename variables

88aecc6

yebukong requested a review from paliwalashish November 25, 2023 03:49

dongjoon-hyun reviewed Nov 25, 2023

View reviewed changes

java/core/src/test/org/apache/orc/impl/TestRecordReaderUtils.java Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Nov 25, 2023

View reviewed changes

java/core/src/test/org/apache/orc/impl/TestRecordReaderUtils.java Outdated Show resolved Hide resolved

dongjoon-hyun added this to the 1.8.7 milestone Nov 25, 2023

dongjoon-hyun requested changes Nov 25, 2023

View reviewed changes

guiyanakuang reviewed Nov 25, 2023

View reviewed changes

yebukong mentioned this pull request Nov 25, 2023

ORC-1530: Rename variables in RecordReaderUtils.ChunkReader#create #1664

Closed

yebukong changed the title ~~ORC-1528: Fix readBytes potential overflow in RecordReaderUtils#create~~ ORC-1528: Fix readBytes potential overflow in RecordReaderUtils.ChunkReader#create Nov 25, 2023

yebukong added 3 commits November 27, 2023 14:14

Revert "rename variables"

a9f325f

This reverts commit 88aecc6.

adjusting code style

c6cc177

adjust validation for reqBytes and readBytes to MAX_ARRAY_SIZE

1869d72

yebukong requested review from dongjoon-hyun and guiyanakuang November 27, 2023 07:02

guiyanakuang reviewed Nov 27, 2023

View reviewed changes

java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java Show resolved Hide resolved

yebukong added 2 commits November 27, 2023 21:09

adjust validation conditions

4f9dfc2

readBytes and reqBytes validation UT

26b3501

yebukong requested a review from guiyanakuang November 27, 2023 13:16

rename the UT methods

0e6c74a

paliwalashish reviewed Nov 27, 2023

View reviewed changes

paliwalashish approved these changes Nov 27, 2023

View reviewed changes

dongjoon-hyun approved these changes Nov 27, 2023

View reviewed changes

dongjoon-hyun closed this in e554765 Nov 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ORC-1528: Fix readBytes potential overflow in RecordReaderUtils.ChunkReader#create #1662

ORC-1528: Fix readBytes potential overflow in RecordReaderUtils.ChunkReader#create #1662

yebukong commented Nov 24, 2023 •

edited

Loading

guiyanakuang commented Nov 24, 2023

paliwalashish Nov 24, 2023

yebukong Nov 24, 2023

dongjoon-hyun Nov 25, 2023 •

edited

Loading

paliwalashish Nov 25, 2023

dongjoon-hyun Nov 25, 2023 •

edited

Loading

dongjoon-hyun commented Nov 25, 2023

guiyanakuang Nov 25, 2023

yebukong Nov 25, 2023

guiyanakuang Nov 25, 2023

guiyanakuang Nov 27, 2023

yebukong Nov 27, 2023

yebukong Nov 28, 2023 •

edited

Loading

yebukong commented Nov 25, 2023 •

edited

Loading

dongjoon-hyun commented Nov 27, 2023

dongjoon-hyun commented Nov 27, 2023

yebukong commented Nov 27, 2023

paliwalashish Nov 27, 2023

yebukong Nov 28, 2023

paliwalashish left a comment

dongjoon-hyun left a comment

dongjoon-hyun commented Nov 27, 2023

dongjoon-hyun commented Nov 27, 2023

yebukong commented Nov 29, 2023 •

edited

Loading

	/**
	* The maximum size of array to allocate, value being the same as {@link java.util.Hashtable},
	* given the fact that the stripe size could be increased to larger value by configuring "orc.stripe.size".
	*/
	private static final int MAX_ARRAY_SIZE = Integer.MAX_VALUE - 8;

ORC-1528: Fix readBytes potential overflow in RecordReaderUtils.ChunkReader#create #1662

ORC-1528: Fix readBytes potential overflow in RecordReaderUtils.ChunkReader#create #1662

Conversation

yebukong commented Nov 24, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

How was this patch tested?

guiyanakuang commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun Nov 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun Nov 25, 2023 • edited Loading

Choose a reason for hiding this comment

dongjoon-hyun commented Nov 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yebukong Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

yebukong commented Nov 25, 2023 • edited Loading

dongjoon-hyun commented Nov 27, 2023

dongjoon-hyun commented Nov 27, 2023

yebukong commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paliwalashish left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Nov 27, 2023

dongjoon-hyun commented Nov 27, 2023

yebukong commented Nov 29, 2023 • edited Loading

yebukong commented Nov 24, 2023 •

edited

Loading

dongjoon-hyun Nov 25, 2023 •

edited

Loading

dongjoon-hyun Nov 25, 2023 •

edited

Loading

yebukong Nov 28, 2023 •

edited

Loading

yebukong commented Nov 25, 2023 •

edited

Loading

yebukong commented Nov 29, 2023 •

edited

Loading