quic: address new coverity warning #48384
Conversation
mhdawson
commented
Jun 7, 2023
mhdawson
commented
- address coverity warning about unitialized variable
- address coverity warning about unitialized variable Signed-off-by: Michael Dawson <[email protected]>
nodejs-github-bot
added
c++
|
Coverity report. There are other reports in recent quic code but will review/address as I have time of other people don't beat me to it. 364 auto& state = BindingData::Get(env);
365 auto params = value.As<Object>();
3. var_decl: Declaring variable options.
366 Options options;
367
368#define SET(name) \
369 SetOption<Session::Options, &Session::Options::name>( \
370 env, &options, params, state.name##_string())
371
CID 318441 (#1 of 1): Uninitialized scalar variable (UNINIT)
4. uninit_use_in_call: Using uninitialized value options. Field options.transport_params.preferred_address_ipv4._M_payload._M_payload is uninitialized when calling SetOption. [[show details](https://scan9.scan.coverity.com/eventId=9078721-4&modelId=9078721-0&fileInstanceId=114681342&filePath=%2Fsrc%2Fquic%2Fsession.cc&fileStart=215&fileEnd=226)]
372 if (!SET(version) || !SET(min_version) || !SET(preferred_address_strategy) ||
373 !SET(transport_params) || !SET(tls_options) ||
374 !SET(application_options) || !SET(qlog)) {
375 return Nothing<Options>();
376 }
377 |
debadree25
added
the
author ready
github-actions
bot
removed
the
request-ci
| @@ -363,7 +363,7 @@ Maybe<Session::Options> Session::Options::From(Environment* env, | |||
|
|
|||
| auto& state = BindingData::Get(env); | |||
| auto params = value.As<Object>(); | |||
| Options options; | |||
| Options options = Options(); | |||
There was a problem hiding this comment.
This seems odd. I might be missing something, but the explicit constructor call Options() should perform the same initialization as the implicit call in Options options, and assignment should not change anything either.
There was a problem hiding this comment.
@tniessen I'm not 100% sure this will fix the warning as I don't know that we can run coverity locally. I think other similar reports have been resolved by a similar fix through. Do you have an alternate suggestion?
There was a problem hiding this comment.
@tniessen I'll commit to looking to make sure it resolved the report after this lands. If it does not I'll figure out some other change that should. I'm thinking that is the easiest way to figure out if it does/does not unless you have another suggestion.
There was a problem hiding this comment.
@mhdawson Sorry, I missed the first ping. This still seems odd to me, especially given that coverity complains about options.transport_params.preferred_address_ipv4._M_payload._M_payload (that is, a field in a class that is not Options), but this PR has the necessary approvals to land and I won't block it.
There was a problem hiding this comment.
@tniessen you were correct. My change did not resolve the Coverity report. Will investigate further to better understand what is being reported.
- address coverity warning about unitialized variable Signed-off-by: Michael Dawson <[email protected]> PR-URL: #48384 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>
|
Landed in ac6f594 |
RafaelGSS
added
dont-land-on-v16.x
dont-land-on-v18.x
|
This PR depends on #47927 which didn't land on v20.x. |
- address coverity warning about unitialized variable Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#48384 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>
- address coverity warning about unitialized variable Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#48384 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>
