Skip to content

src: ccm cipher update with zero-length dataView#62343

Open
mertcanaltin wants to merge 1 commit intonodejs:mainfrom
mertcanaltin:mert/fix/ccm-zero-length-dataview
Open

src: ccm cipher update with zero-length dataView#62343
mertcanaltin wants to merge 1 commit intonodejs:mainfrom
mertcanaltin:mert/fix/ccm-zero-length-dataview

Conversation

@mertcanaltin
Copy link
Member

added fallback stack_storage for null scenario

for:#62342

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 19, 2026
@mertcanaltin mertcanaltin force-pushed the mert/fix/ccm-zero-length-dataview branch from 664507a to 88f8f03 Compare March 19, 2026 20:58
@codecov
Copy link

codecov bot commented Mar 19, 2026

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.68%. Comparing base (0998c37) to head (88f8f03).

Files with missing lines Patch % Lines
src/util-inl.h 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62343      +/-   ##
==========================================
- Coverage   91.60%   89.68%   -1.92%     
==========================================
  Files         337      676     +339     
  Lines      140741   206692   +65951     
  Branches    21797    39585   +17788     
==========================================
+ Hits       128923   185378   +56455     
- Misses      11594    13445    +1851     
- Partials      224     7869    +7645     
Files with missing lines Coverage Δ
src/util-inl.h 82.69% <75.00%> (ø)

... and 458 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@panva panva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With ArrayBufferViewContents being used broadly throughout node's codebase I wonder if we can be sure this is entirely without side effects. I cannot assess that.

That being said, it does fix the particular node:crypto issue reported.

@mertcanaltin
Copy link
Member Author

mertcanaltin commented Mar 20, 2026

With ArrayBufferViewContents being used broadly throughout node's codebase I wonder if we can be sure this is entirely without side effects. I cannot assess that.

That being said, it does fix the particular node:crypto issue reported.

I went through all 19 files that use ArrayBufferViewContents, every usage passes data() alongside length(), and none dereferences the pointer when length is 0. Currently data_ can already be null in this scenario, this fix just ensures it's a valid pointer instead, so it's strictly safer than the current behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants