Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Buffer.toString() cannot handle large indices #52298

Closed
imyp92 opened this issue Apr 1, 2024 · 5 comments · Fixed by #54553
Closed

Buffer.toString() cannot handle large indices #52298

imyp92 opened this issue Apr 1, 2024 · 5 comments · Fixed by #54553
Labels
buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs.

Comments

@imyp92
Copy link

imyp92 commented Apr 1, 2024

Version

14.17.3

Platform

23.4.0 Darwin Kernel Version 23.4.0

Subsystem

No response

What steps will reproduce the bug?

let buffer = Buffer.alloc(2279415336);

let res = buffer.toString('utf8', 2147483648, 2147483700); // 2^32 - 1 < start

// buffer.js:605
//     slice: (buf, start, end) => buf.utf8Slice(start, end),
                                    ^

// RangeError: Index out of range

How often does it reproduce? Is there a required condition?

everytime

What is the expected behavior? Why is that the expected behavior?

Buffer.toString() should be able to handle buffers smaller than kMaxLength.

What do you see instead?

index out of range error

Additional information

The bitwise or assignment (|=) operation of Buffer.toString() seems to be the cause of the error. If start or end parameter greater than INT_MAX is passed, the value changes to a negative number, resulting in an index out of range error.

@imyp92 imyp92 changed the title Buffer.toString() cannot handle large indexes Buffer.toString() cannot handle large indices Apr 1, 2024
@Malikrehman00107
Copy link

The issue you're encountering is due to JavaScript's handling of integers beyond Number.MAX_SAFE_INTEGER (which is 2^53 - 1), resulting in the values being interpreted as negative when passed to Buffer.toString().

make sure that the start and end parameters passed to Buffer.toString() are within the range of safe integers. You can achieve this by performing bounds checking before calling Buffer.toString().

@imyp92
Copy link
Author

imyp92 commented Apr 1, 2024

The issue you're encountering is due to JavaScript's handling of integers beyond Number.MAX_SAFE_INTEGER (which is 2^53 - 1), resulting in the values being interpreted as negative when passed to Buffer.toString().

make sure that the start and end parameters passed to Buffer.toString() are within the range of safe integers. You can achieve this by performing bounds checking before calling Buffer.toString().

Sorry for being made an unclear comment on example code. I wrote INT_MAX < start but I think it would be correct to express it as 2^32-1.
This problem occurs when using a value greater than 2^31-1 as offsets(start or end). And this is smaller than the kMaxLength constant, which is the maximum size of buffer allowed by Buffer.js (in 64-bit architecture, 2^32). so I think this case should be handled by Buffer.toString().

@Malikrehman00107
Copy link

I see !

you may need to perform bounds checking before calling Buffer.toString() to ensure that the offsets are within the range of safe integers. This will help prevent the index out of range errors until a fix or enhancement is made to Buffer.toString().

Rest i think Node Js internal need to have a look on this.

@targos
Copy link
Member

targos commented Apr 1, 2024

This should fix it in a non-breaking way:

diff --git a/lib/buffer.js b/lib/buffer.js
index a8d07342e15..904f1fff6b3 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -820,12 +820,12 @@ Buffer.prototype.toString = function toString(encoding, start, end) {
   else if (start >= len)
     return '';
   else
-    start |= 0;
+    start = MathTrunc(start) || 0;

   if (end === undefined || end > len)
     end = len;
   else
-    end |= 0;
+    end = MathTrunc(end) || 0;

   if (end <= start)
     return '';

@Malikrehman00107
Copy link

Thank you @targos !

@VoltrexKeyva VoltrexKeyva added the buffer Issues and PRs related to the buffer subsystem. label Apr 3, 2024
@RedYetiDev RedYetiDev added the confirmed-bug Issues with confirmed bugs. label Apr 28, 2024
nodejs-github-bot pushed a commit that referenced this issue Sep 6, 2024
Co-authored-by: Michaël Zasso <targos@protonmail.com>
PR-URL: #54553
Fixes: #52298
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
aduh95 pushed a commit that referenced this issue Sep 12, 2024
Co-authored-by: Michaël Zasso <targos@protonmail.com>
PR-URL: #54553
Fixes: #52298
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
jazelly added a commit to jazelly/node that referenced this issue Oct 2, 2024
Co-authored-by: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#54553
Fixes: nodejs#52298
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
jazelly added a commit to jazelly/node that referenced this issue Oct 2, 2024
Co-authored-by: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#54553
Fixes: nodejs#52298
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
jazelly added a commit to jazelly/node that referenced this issue Oct 3, 2024
Co-authored-by: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#54553
Fixes: nodejs#52298
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
Co-authored-by: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#54553
Fixes: nodejs#52298
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants