child_process: validate arguments for null bytes #44782
Merged
+322
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
c74b77f
to
67903b0
Compare
This comment was marked as outdated.
This comment was marked as outdated.
bnoordhuis
reviewed
Sep 25, 2022
This was referenced Sep 26, 2022
This change adds validation to reject an edge case where the child_process API argument strings might contain null bytes somewhere in between. Such strings were being silently truncated before, so throwing an error should prevent misuses of this API. Fixes: nodejs#44768 Signed-off-by: Darshan Sen <raisinten@gmail.com>
67903b0
to
a6bb1c4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This was referenced Oct 5, 2022
|
cc @nodejs/child_process in case anyone else also wants to take a look. |
20 tasks
|
@ZYSzys I would say no because code that previously tried to pass string arguments with null bytes was already broken because the strings were silently getting truncated at the first null byte. This validation would make it easier for users to know where things went wrong. |
This was referenced Oct 8, 2022
jasnell
approved these changes
Oct 10, 2022
This comment was marked as outdated.
This comment was marked as outdated.
|
@bnoordhuis @ZYSzys would y'all like to take another look? |
|
Landed in 91dbd8b |
RafaelGSS
pushed a commit
that referenced
this pull request
Nov 1, 2022
This change adds validation to reject an edge case where the child_process API argument strings might contain null bytes somewhere in between. Such strings were being silently truncated before, so throwing an error should prevent misuses of this API. Fixes: #44768 Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #44782 Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
RafaelGSS
pushed a commit
that referenced
this pull request
Nov 10, 2022
This change adds validation to reject an edge case where the child_process API argument strings might contain null bytes somewhere in between. Such strings were being silently truncated before, so throwing an error should prevent misuses of this API. Fixes: #44768 Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #44782 Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
This change adds validation to reject an edge case where the
child_processAPI argument strings might contain null bytes somewhere in between. Such strings were being silently truncated before, so throwing an error should prevent misuses of this API.Fixes: #44768
Signed-off-by: Darshan Sen raisinten@gmail.com