enhance memory safety in DNS parser by adding bounds checks#1576
enhance memory safety in DNS parser by adding bounds checks#1576rootvector2 wants to merge 3 commits into
Conversation
phoddie
left a comment
phoddie
left a comment
There was a problem hiding this comment.
Thanks for the PR to harden the DNS parsing. Very much appreciated. I left a few notes for changes. In particular, it is important to keep in mind that the garbage collector in the XS engine compacts memory, which can invalidate pointers held in C.
Before we can merge this PR, we need you to complete our Contributor License Agreement - choose either the Individual or corporate, as appropriate.
…ds checks - Move xsNewArray() before getPacket() in parseQname so GC triggered by xsNewArray cannot invalidate the packetStart/packetEnd pointers - Re-fetch packetStart and packetEnd after xsmcSetStringBuffer and xsCall1 in parseQname loop since both can trigger GC and move memory - Consolidate inner name-label traversal from 'while (true)' with multiple early-return guards to 'while (position < packetEnd)' in both getQuestionByIndex and getAnswerByIndex, relying on the existing post-loop bounds check to catch malformed packets (per phoddie review)
|
I’ve completed the CLA (individual). Please let me know where to send the signed copy. |
Please send a scan (preferably PDF) by email to info@moddable.com. Thank you. |
ok Sent |
|
Thanks – we received the CLA. There is an inconsistency. Most of the changes throw when invalid data is detected. I think that's the right behavior. The one exception is the |
|
I agree that silently exiting in TXT parsing can lead to incomplete or misleading data being returned to the script. It should behave consistently with the rest of the parser and throw on invalid input. I’ll update the TXT parsing to throw an error instead of breaking. |
|
Great, thank you. We should have everything needed to merge this as part of our upcoming April release. |
|
Thanks, really appreciate the review and guidance! If there’s anything else I can help with or take up, feel free to let me know. |
|
Happy to have the help! I am curious... the parsing errors are real but a bit obscure. How did you stumble over them in the first place? |
|
Honestly, I was just going through the DNS parser with a focus on how it handles malformed inputs and memory safety. I noticed a few TODO comments and some places where bounds checks were missing, especially around label parsing and compression pointers. That got me curious, so I followed those paths and tried to think through how malformed packets might behave, which is how I ended up spotting those cases. |
|
Very cool! If you are looking for a good challenge, staying within the DNS module the serializer ( |
2 participants
Fix memory-safety issues in DNS parsing (bounds checks, pointer validation, integer underflow) to prevent out-of-bounds reads and malformed packet handling issues.