src: handle null backing store in ArrayBufferViewContents::Read#62343
src: handle null backing store in ArrayBufferViewContents::Read#62343mertcanaltin wants to merge 1 commit intonodejs:mainfrom
Conversation
664507a to
88f8f03
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62343 +/- ##
==========================================
- Coverage 91.60% 89.69% -1.91%
==========================================
Files 337 676 +339
Lines 140741 206696 +65955
Branches 21797 39573 +17776
==========================================
+ Hits 128923 185397 +56474
- Misses 11594 13443 +1849
- Partials 224 7856 +7632
🚀 New features to boost your workflow:
|
panva
left a comment
There was a problem hiding this comment.
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. |
Very well, can you update the commit message to |
88f8f03 to
8104939
Compare
sure, edited thanks |
| auto buf = abv->Buffer(); | ||
| data_ = buf->Data() != nullptr | ||
| ? static_cast<T*>(buf->Data()) + abv->ByteOffset() | ||
| : stack_storage_; |
There was a problem hiding this comment.
Nit:
| auto buf = abv->Buffer(); | |
| data_ = buf->Data() != nullptr | |
| ? static_cast<T*>(buf->Data()) + abv->ByteOffset() | |
| : stack_storage_; | |
| auto buf_data = abv->Buffer()->Data(); | |
| data_ = buf_data != nullptr | |
| ? static_cast<T*>(buf_data) + abv->ByteOffset() | |
| : stack_storage_; |
added fallback stack_storage for null scenario
for:#62342