Copilot commented on code in PR #378:
URL: https://github.com/apache/arrow-js/pull/378#discussion_r2771798989
##########
src/ipc/message.ts:
##########
@@ -150,6 +174,10 @@ export class AsyncMessageReader implements
AsyncIterableIterator<Message> {
const buf = await this.source.read(PADDING);
const bb = buf && new ByteBuffer(buf);
const len = bb?.readInt32(0) || 0;
+ // Guard against corrupted files with unreasonable metadata lengths
+ if (len < 0 || len > MAX_METADATA_LENGTH) {
Review Comment:
The validation condition `len < 0` will break the continuation indicator
logic used for Arrow IPC format v0.15+. According to the comment on lines
132-134, a value of -1 is a valid continuation indicator per ARROW-6313. This
validation should exclude -1 from being treated as an error. Consider changing
the condition to `len < -1` or `(len < 0 && len !== -1)`.
```suggestion
if ((len < 0 && len !== -1) || len > MAX_METADATA_LENGTH) {
```
##########
src/ipc/message.ts:
##########
@@ -59,6 +73,9 @@ export class MessageReader implements
IterableIterator<Message> {
}
public readMessageBody(bodyLength: number): Uint8Array {
if (bodyLength <= 0) { return new Uint8Array(0); }
+ if (bodyLength > MAX_BODY_LENGTH) {
+ throw new Error(`Message body length ${bodyLength} exceeds maximum
allowed size of ${MAX_BODY_LENGTH} bytes. The file may be corrupted.`);
+ }
Review Comment:
The new validation logic for MAX_BODY_LENGTH lacks test coverage. Consider
adding test cases that verify the error is thrown when bodyLength exceeds
MAX_BODY_LENGTH (2GB), and that valid body lengths just under the limit still
work correctly. This is especially important given that this fix addresses a
critical hanging issue reported in issue #315.
##########
src/ipc/message.ts:
##########
@@ -128,6 +149,9 @@ export class AsyncMessageReader implements
AsyncIterableIterator<Message> {
}
public async readMessageBody(bodyLength: number): Promise<Uint8Array> {
if (bodyLength <= 0) { return new Uint8Array(0); }
+ if (bodyLength > MAX_BODY_LENGTH) {
+ throw new Error(`Message body length ${bodyLength} exceeds maximum
allowed size of ${MAX_BODY_LENGTH} bytes. The file may be corrupted.`);
+ }
Review Comment:
The new validation logic for MAX_BODY_LENGTH lacks test coverage. Consider
adding test cases that verify the error is thrown when bodyLength exceeds
MAX_BODY_LENGTH (2GB), and that valid body lengths just under the limit still
work correctly.
##########
src/ipc/message.ts:
##########
@@ -81,6 +98,10 @@ export class MessageReader implements
IterableIterator<Message> {
const buf = this.source.read(PADDING);
const bb = buf && new ByteBuffer(buf);
const len = bb?.readInt32(0) || 0;
+ // Guard against corrupted files with unreasonable metadata lengths
+ if (len < 0 || len > MAX_METADATA_LENGTH) {
Review Comment:
The validation condition `len < 0` will break the continuation indicator
logic used for Arrow IPC format v0.15+. According to the comment on lines 56-58
(and 132-134 for async), a value of -1 is a valid continuation indicator per
ARROW-6313. This validation should exclude -1 from being treated as an error.
Consider changing the condition to `len < -1` or `(len < 0 && len !== -1)`.
```suggestion
if (len < -1 || len > MAX_METADATA_LENGTH) {
```
##########
src/ipc/message.ts:
##########
@@ -150,6 +174,10 @@ export class AsyncMessageReader implements
AsyncIterableIterator<Message> {
const buf = await this.source.read(PADDING);
const bb = buf && new ByteBuffer(buf);
const len = bb?.readInt32(0) || 0;
+ // Guard against corrupted files with unreasonable metadata lengths
+ if (len < 0 || len > MAX_METADATA_LENGTH) {
+ throw new Error(`Invalid metadata length ${len}. The file may be
corrupted.`);
+ }
Review Comment:
The new validation logic for MAX_METADATA_LENGTH lacks test coverage.
Consider adding test cases that verify the error is thrown when metadata length
exceeds MAX_METADATA_LENGTH (256MB) or is negative (excluding the valid -1
continuation indicator), and that valid metadata lengths still work correctly.
##########
src/ipc/message.ts:
##########
@@ -81,6 +98,10 @@ export class MessageReader implements
IterableIterator<Message> {
const buf = this.source.read(PADDING);
const bb = buf && new ByteBuffer(buf);
const len = bb?.readInt32(0) || 0;
+ // Guard against corrupted files with unreasonable metadata lengths
+ if (len < 0 || len > MAX_METADATA_LENGTH) {
+ throw new Error(`Invalid metadata length ${len}. The file may be
corrupted.`);
+ }
Review Comment:
The new validation logic for MAX_METADATA_LENGTH lacks test coverage.
Consider adding test cases that verify the error is thrown when metadata length
exceeds MAX_METADATA_LENGTH (256MB) or is negative (excluding the valid -1
continuation indicator), and that valid metadata lengths still work correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
