Conversation
ValoChet
changed the title
|
There is a critical integer underflow vulnerability in the NativeString class that leads to a heap buffer overflow. This occurs because the pool allocation check does not account for the 8-byte alignment padding applied when committing the offset. |
|
Also to be frank with you, this release https://github.com/uNetworking/uWebSockets.js/releases/tag/v20.63.0 has put me very wary of your PRs, esp. touching NativeString. |
I know, AI flagged that for me too, but it can't happen if the pool size is a multiple of 8. That's why I added a comment about it. Regarding the actual implementation, it may contain an overflow issue in About the #1262 ValueView event, I am sorry for what happened, I misread the V8 documentation (which is hard to find and AI was not as powerful as now) and thought strings were given as UTF-8 or UTF-16. But to be fair my PR was in draft status and I would have liked more time to test and validate it before it was merged. |
2 participants
String pooling optimization:
For pooled strings, we can avoid calculating
Utf8Length()by using the worst-case allocation size:2 * sizein worst case3 * sizein worst caseIf this worst-case size fits in the pool buffer, we can call
WriteUtf8()directly.It writes the UTF-8 data into the pool and returns the exact number of bytes written.
The pool offset is then advanced by the actual number of bytes used.
This should cover most strings thanks to the generous 128 KB pool.
If the string is too large, it fallback calculating the exact size.
ArrayBuffer Data() function:
Since Node.js 20 the new
ArrayBuffer::Data()function can replaceArrayBuffer::GetBackingStore()->Data().It removes the need to instantiate the
shared_ptrand increase performance.Test case: 10 x
writeHeader(32 bytes, 32 bytes):