You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Divyanshu-s13 (via GitHub)" <gi...@apache.org> on 2026/02/05 15:12:15 UTC
[PR] fix: prevent tableFromIPC from hanging on corrupted files#315 [arrow-js]
Divyanshu-s13 opened a new pull request, #378:
URL: https://github.com/apache/arrow-js/pull/378
What's Changed
Fixed an issue where [tableFromIPC()] could hang indefinitely when reading corrupted IPC files. The problem occurred when a corrupted file contained invalid metadata length or body length values, causing the reader to wait forever trying to read bytes that don't exist.
Added validation to reject unreasonably large metadata lengths (>256MB) and body lengths (>2GB), throwing a clear error message instead of hanging.
Closes #315.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: prevent tableFromIPC from hanging on corrupted files [arrow-js]
Posted by "Divyanshu-s13 (via GitHub)" <gi...@apache.org>.
Divyanshu-s13 commented on code in PR #378:
URL: https://github.com/apache/arrow-js/pull/378#discussion_r2774720587
##########
src/ipc/message.ts:
##########
@@ -29,6 +29,20 @@ import { ArrowJSON, ArrowJSONLike, ITERATOR_DONE, FileHandle } from '../io/inter
/** @ignore */ const invalidMessageMetadata = (expected: number, actual: number) => `Expected to read ${expected} metadata bytes, but only read ${actual}.`;
/** @ignore */ const invalidMessageBodyLength = (expected: number, actual: number) => `Expected to read ${expected} bytes for message body, but only read ${actual}.`;
+/**
+ * Maximum allowed metadata length (256 MB). This is a safeguard against corrupted
+ * files that could cause the reader to hang or consume excessive memory.
+ * @ignore
+ */
+const MAX_METADATA_LENGTH = 256 * 1024 * 1024;
+
+/**
+ * Maximum allowed message body length (2 GB). This is a safeguard against corrupted
+ * files that could cause the reader to hang or consume excessive memory.
+ * @ignore
+ */
+const MAX_BODY_LENGTH = 2 * 1024 * 1024 * 1024;
+
Review Comment:
> Yeah these are wrong, Arrow tables can be larger than this. It'd be better to figure out why your IPC stream is corrupted when it's written.
Thank you for the feedback! You're right - I've reverted the size limits as they were too restrictive for valid Arrow files.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: prevent tableFromIPC from hanging on corrupted files [arrow-js]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on code in PR #378:
URL: https://github.com/apache/arrow-js/pull/378#discussion_r2875357204
##########
src/ipc/message.ts:
##########
@@ -29,6 +29,20 @@ import { ArrowJSON, ArrowJSONLike, ITERATOR_DONE, FileHandle } from '../io/inter
/** @ignore */ const invalidMessageMetadata = (expected: number, actual: number) => `Expected to read ${expected} metadata bytes, but only read ${actual}.`;
/** @ignore */ const invalidMessageBodyLength = (expected: number, actual: number) => `Expected to read ${expected} bytes for message body, but only read ${actual}.`;
+/**
+ * Maximum allowed metadata length (256 MB). This is a safeguard against corrupted
+ * files that could cause the reader to hang or consume excessive memory.
+ * @ignore
+ */
+const MAX_METADATA_LENGTH = 256 * 1024 * 1024;
+
+/**
+ * Maximum allowed message body length (2 GB). This is a safeguard against corrupted
+ * files that could cause the reader to hang or consume excessive memory.
+ * @ignore
+ */
+const MAX_BODY_LENGTH = 2 * 1024 * 1024 * 1024;
+
Review Comment:
We can't add a timeout in Arrow since we're just processing a stream the user gives us. We don't have enough context to know why a read call might take a certain amount of time. Is the file on a slow NFS drive? Is it a `fetch()` to a dynamic resource and the the next batch hasn't been computed yet? Is the user's connection just slow?
That said, you can add a timeout before/after Arrow's transform stream to either terminate/cancel based on your users' expected behavior.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: prevent tableFromIPC from hanging on corrupted files [arrow-js]
Posted by "Divyanshu-s13 (via GitHub)" <gi...@apache.org>.
Divyanshu-s13 commented on PR #378:
URL: https://github.com/apache/arrow-js/pull/378#issuecomment-3861072436
> Why do we need max size check for this fix? Can we detect "no more data" instead of max size check?
I have fixed this.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: prevent tableFromIPC from hanging on corrupted files#315 [arrow-js]
Posted by "Divyanshu-s13 (via GitHub)" <gi...@apache.org>.
Divyanshu-s13 commented on PR #378:
URL: https://github.com/apache/arrow-js/pull/378#issuecomment-3854534014
@kou I’ve fixed the issue. Could you please merge it?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: prevent tableFromIPC from hanging on corrupted files [arrow-js]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on code in PR #378:
URL: https://github.com/apache/arrow-js/pull/378#discussion_r2875357204
##########
src/ipc/message.ts:
##########
@@ -29,6 +29,20 @@ import { ArrowJSON, ArrowJSONLike, ITERATOR_DONE, FileHandle } from '../io/inter
/** @ignore */ const invalidMessageMetadata = (expected: number, actual: number) => `Expected to read ${expected} metadata bytes, but only read ${actual}.`;
/** @ignore */ const invalidMessageBodyLength = (expected: number, actual: number) => `Expected to read ${expected} bytes for message body, but only read ${actual}.`;
+/**
+ * Maximum allowed metadata length (256 MB). This is a safeguard against corrupted
+ * files that could cause the reader to hang or consume excessive memory.
+ * @ignore
+ */
+const MAX_METADATA_LENGTH = 256 * 1024 * 1024;
+
+/**
+ * Maximum allowed message body length (2 GB). This is a safeguard against corrupted
+ * files that could cause the reader to hang or consume excessive memory.
+ * @ignore
+ */
+const MAX_BODY_LENGTH = 2 * 1024 * 1024 * 1024;
+
Review Comment:
We can't add a timeout in Arrow since we're just processing a stream the library user gives us. We don't have enough context to know why a read call might take a certain amount of time. Is the file on a slow NFS drive? Is it a `fetch()` to a dynamic resource and the the next batch hasn't been computed yet? Is the user's connection just slow?
That said, you can add a timeout before/after Arrow's transform stream to either terminate/cancel based on your users' expected behavior.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: prevent tableFromIPC from hanging on corrupted files [arrow-js]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #378:
URL: https://github.com/apache/arrow-js/pull/378#issuecomment-3857224186
Why do we need max size check for this fix?
Can we detect "no more data" instead of max size check?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: prevent tableFromIPC from hanging on corrupted files [arrow-js]
Posted by "james-sift (via GitHub)" <gi...@apache.org>.
james-sift commented on code in PR #378:
URL: https://github.com/apache/arrow-js/pull/378#discussion_r2874207463
##########
src/ipc/message.ts:
##########
@@ -29,6 +29,20 @@ import { ArrowJSON, ArrowJSONLike, ITERATOR_DONE, FileHandle } from '../io/inter
/** @ignore */ const invalidMessageMetadata = (expected: number, actual: number) => `Expected to read ${expected} metadata bytes, but only read ${actual}.`;
/** @ignore */ const invalidMessageBodyLength = (expected: number, actual: number) => `Expected to read ${expected} bytes for message body, but only read ${actual}.`;
+/**
+ * Maximum allowed metadata length (256 MB). This is a safeguard against corrupted
+ * files that could cause the reader to hang or consume excessive memory.
+ * @ignore
+ */
+const MAX_METADATA_LENGTH = 256 * 1024 * 1024;
+
+/**
+ * Maximum allowed message body length (2 GB). This is a safeguard against corrupted
+ * files that could cause the reader to hang or consume excessive memory.
+ * @ignore
+ */
+const MAX_BODY_LENGTH = 2 * 1024 * 1024 * 1024;
+
Review Comment:
Is there a way to add a timeout here? A function that never returns and has no way to be interrupted isn't something we can use in production.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: prevent tableFromIPC from hanging on corrupted files [arrow-js]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: prevent tableFromIPC from hanging on corrupted files [arrow-js]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on code in PR #378:
URL: https://github.com/apache/arrow-js/pull/378#discussion_r2772544262
##########
src/ipc/message.ts:
##########
@@ -29,6 +29,20 @@ import { ArrowJSON, ArrowJSONLike, ITERATOR_DONE, FileHandle } from '../io/inter
/** @ignore */ const invalidMessageMetadata = (expected: number, actual: number) => `Expected to read ${expected} metadata bytes, but only read ${actual}.`;
/** @ignore */ const invalidMessageBodyLength = (expected: number, actual: number) => `Expected to read ${expected} bytes for message body, but only read ${actual}.`;
+/**
+ * Maximum allowed metadata length (256 MB). This is a safeguard against corrupted
+ * files that could cause the reader to hang or consume excessive memory.
+ * @ignore
+ */
+const MAX_METADATA_LENGTH = 256 * 1024 * 1024;
+
+/**
+ * Maximum allowed message body length (2 GB). This is a safeguard against corrupted
+ * files that could cause the reader to hang or consume excessive memory.
+ * @ignore
+ */
+const MAX_BODY_LENGTH = 2 * 1024 * 1024 * 1024;
+
Review Comment:
Yeah these are wrong, Arrow tables can be larger than this. It'd be better to figure out why your IPC stream is corrupted when it's written.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org