apache / apache/lucene

The bytes parameter of the copy method is not carefully checked. [LUCENE-8536]

Open
#9,582 0 comments 0 reactions 0 assignees View on GitHub
affects-version:trunk legacy-jira-priority:Major tool:build type:bug
Dominant language
Java
Stars
3.6k
Forks
1.4k
Avg merge
2d 11h
Merged PRs (30d)
88

Description

The copy method of the PagedBytes class is as follow:

 

 

```java
public void copy(BytesRef bytes, BytesRef out) {
int left = blockSize - upto;
if (bytes.length > left || currentBlock==null) {
if (currentBlock != null) {
addBlock(currentBlock);
didSkipBytes = true;
}
currentBlock = new byte[blockSize];
upto = 0;
left = blockSize;
assert bytes.length <= blockSize;
// TODO: we could also support variable block sizes
}

out.bytes = currentBlock;
out.offset = upto;
out.length = bytes.length;

System.arraycopy(bytes.bytes, bytes.offset, currentBlock, upto, bytes.length);
upto += bytes.length;
}
```

The method does not throw exceptions for illegal inputs. In the same class, the copyUsingLengthPrefix method checks the input value"

 

 

 

```java
public long copyUsingLengthPrefix(BytesRef bytes) {
if (bytes.length >= 32768) {
throw new IllegalArgumentException("max length is 32767 (got " + bytes.length + ")");
}

if (upto + bytes.length + 2 > blockSize) {
if (bytes.length + 2 > blockSize) {
throw new IllegalArgumentException("block size " + blockSize + " is too small to store length " + bytes.length + " bytes");
}
if (currentBlock != null) {
addBlock(currentBlock);
}
currentBlock = new byte[blockSize];
upto = 0;
}

final long pointer = getPointer();

if (bytes.length < 128) {
currentBlock[upto++] = (byte) bytes.length;
} else {
currentBlock[upto++] = (byte) (0x80 | (bytes.length >> 8));
currentBlock[upto++] = (byte) (bytes.length & 0xff);
}
System.arraycopy(bytes.bytes, bytes.offset, currentBlock, upto, bytes.length);
upto += bytes.length;

return pointer;
}
```

I understand that in the first method, 

```java
assert bytes.length <= blockSize;
```

checks whether the length of the bytes is too large. However,  the method does not check blockSize either. As a result, the length of the bytes can still be overflowed, if blockSize is too large.  In addition, the second method also checks whether 

```java
bytes.length + 2 > blockSize
```

Shall the first method also checks the requirement?

---
Migrated from [LUCENE-8536](https://issues.apache.org/jira/browse/LUCENE-8536) by Hao Zhong, updated Apr 14 2019

Contributor guide

Open the contributing guide

Research direction

Start at the PagedBytes.copy method and compare its input handling with copyUsingLengthPrefix; inspect callers to understand the blockSize assumptions. The payload names no file or test, so locate relevant coverage and confirm the intended behavior for inputs that exceed the block size, including that illegal lengths are rejected without overflow.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.