Skip to content

buffer: remove lines setting indexes to integer value from Blob.slice - #52588

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
kylo5aby:blob
May 6, 2024
Merged

nodejs-github-bot merged 1 commit into
nodejs:mainfrom
kylo5aby:blob

Conversation

@kylo5aby

@kylo5aby kylo5aby commented Apr 19, 2024 •

Copy link
Copy Markdown
Contributor

Since blob allows maximum kLength of 2GB, the indexes maybe out of signed integer range.
remove lines setting indexes to integer value from Blob.slice.
they will be checked in Cpp side(Blob::ToSlice) if they were not in unsigned integer range.
Refs: #52585

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 19, 2024
@kylo5aby
kylo5aby force-pushed the blob branch 2 times, most recently from d31f4f4 to a00470f Compare April 22, 2024 01:49
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Apr 24, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 24, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@kylo5aby

Copy link
Copy Markdown
Contributor Author

hi @aduh95 , I see an error related to test-error-sedes.js in CI. In the test cases I added, I created a potentially time-consuming large Buffer and placed it in the pummel directory, and skipped the test for 32-bit platforms just like other types of test cases. I believe my changes shouldn't affect test-error-sedes. Could you take a look?
I
image

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@aduh95 aduh95 added the commit-queue PRs queued for automated landing through the Commit Queue. label May 6, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label May 6, 2024
@nodejs-github-bot
nodejs-github-bot merged commit bf8afe7 into nodejs:main May 6, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in bf8afe7

targos pushed a commit that referenced this pull request May 8, 2024
PR-URL: #52588
Refs: #52585
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
PR-URL: #52588
Refs: #52585
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024
PR-URL: nodejs#52588
Refs: nodejs#52585
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Oct 22, 2026
Blob sizes cross between C++ and JavaScript as uint32 values, while the
underlying DataQueue tracks them as uint64. openAsBlob() on a 5 GiB
file therefore returned a Blob of size 1073741824.

Reporting the real size is not enough on its own. The inbound direction
assumes a uint32 as well, so a correct size reaches CHECK(IsUint32())
in ToSlice() and StoreDataObject() and aborts blob.slice() and
URL.createObjectURL(). Pass the sizes as doubles and keep them as
uint64_t in C++. Everything below this boundary is already 64-bit, so
this only stops the narrowing at the edge.

Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
PR-URL: #66207
Fixes: #52585
Refs: #52588
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants