-
Notifications
You must be signed in to change notification settings - Fork 30.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
src: add args validation method #56487
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
34769a1
to
e0af101
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56487 +/- ##
==========================================
+ Coverage 88.52% 89.12% +0.60%
==========================================
Files 660 662 +2
Lines 190900 191576 +676
Branches 36628 36871 +243
==========================================
+ Hits 168995 170743 +1748
+ Misses 15090 13701 -1389
- Partials 6815 7132 +317
|
b4c8fd8
to
8bfc0fb
Compare
src/tcp_wrap.cc
Outdated
CHECK(args[2]->IsUint32()); | ||
Environment* env = Environment::GetCurrent(args); | ||
int backlog; | ||
if (!args[2]->Int32Value(env->context()).To(&backlog)) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i made an update here, so i think we should validate here instead of throwing args error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you have this setting to backlog as an int32 but we use it as port below, cast as a uint32 then converted to an int with port below. Where is the term backlog coming from here? either way, should make this consistent here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, i think the backlog should be a "port" here, and I think we don't need cast again since
int port;
if (!args[2]->Int32Value(env->context()).To(&port)) return;
already casted to int right (?)
and we don't need this anymore
int port = static_cast<int>(args[2].As<Uint32>()->Value());
8bfc0fb
to
5db5ac6
Compare
Problem:
When fewer than three arguments are passed or if the third argument is not of type Uint32, the program attempts to access invalid memory or cast incorrect types. This leads to undefined behavior, including potential crashes
[1] 2385420 IOT instruction (core dumped) node
and not clear what does it meanSo I think we need to validate the args first before we do some assertions below. we can also do same thing in another place that doesn't have the validator
ref issue: #56367