Skip to content
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.

Sign up for GitHub

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

Jump to bottom

inspector: fix Coverity defects #12272

Merged
merged 0 commits into from
Apr 11, 2017
Merged

inspector: fix Coverity defects #12272

merged 0 commits into from
Apr 11, 2017

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Apr 7, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector: minor fixes

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x labels Apr 7, 2017
TimothyGu
TimothyGu reviewed Apr 7, 2017
View reviewed changes
src/inspector_io.cc Outdated
@@ -236,6 +236,7 @@ void InspectorIo::WriteCbIO(uv_async_t* async) {
template<typename Transport>
void InspectorIo::WorkerRunIO() {
uv_loop_t loop;
loop.data = nullptr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to libuv docs, this should be initialized in the next line with uv_loop_init():

void* uv_loop_t.data
Space for user-defined arbitrary data. libuv does not use this field. libuv does, however, initialize it to NULL in uv_loop_init(), and it poisons the value (on debug builds) on uv_loop_close().

Copy link
Contributor Author

@eugeneo eugeneo Apr 7, 2017 •
edited by TimothyGu
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think Coverity is aware of the fact...

CID 166789 (# 1 of 1): Uninitialized pointer read (UNINIT)
2. uninit_use_in_call: Using uninitialized value loop.data when calling uv_loop_init."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I’m missing something obvious, the libuv docs are lying:

node/deps/uv/src/unix/loop.c

Lines 36 to 38 in 2d3d4cc

saved_data = loop->data;
memset(loop, 0, sizeof(*loop));
loop->data = saved_data;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But not for long: libuv/libuv#1299

Alternative syntax: uv_loop_t loop = uv_loop_t() (or auto loop = uv_loop_t() if you don't like to repeat yourself), that zeroes out all fields.

@mscdex mscdex added the inspector Issues and PRs related to the V8 inspector protocol label Apr 7, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented Apr 7, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/7267/

addaleax
addaleax approved these changes Apr 9, 2017
View reviewed changes
src/inspector_io.cc Outdated
@@ -236,6 +236,7 @@ void InspectorIo::WriteCbIO(uv_async_t* async) {
template<typename Transport>
void InspectorIo::WorkerRunIO() {
uv_loop_t loop;
loop.data = nullptr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I’m missing something obvious, the libuv docs are lying:

node/deps/uv/src/unix/loop.c

Lines 36 to 38 in 2d3d4cc

saved_data = loop->data;
memset(loop, 0, sizeof(*loop));
loop->data = saved_data;

bnoordhuis
bnoordhuis reviewed Apr 11, 2017
View reviewed changes
src/inspector_io.cc Outdated
@@ -236,6 +236,7 @@ void InspectorIo::WriteCbIO(uv_async_t* async) {
template<typename Transport>
void InspectorIo::WorkerRunIO() {
uv_loop_t loop;
loop.data = nullptr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But not for long: libuv/libuv#1299

Alternative syntax: uv_loop_t loop = uv_loop_t() (or auto loop = uv_loop_t() if you don't like to repeat yourself), that zeroes out all fields.

src/inspector_io.cc Outdated
if (0 == uv_fs_realpath(&loop, &req, script_name_.c_str(), nullptr))
req.ptr = nullptr;
err = uv_fs_realpath(&loop, &req, script_name_.c_str(), nullptr);
if (0 == err && req.ptr != nullptr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nullptr check shouldn't be necessary but perhaps coverity isn't smart enough to figure that out... I'd turn it into a CHECK_NE(req.ptr, nullptr).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

bnoordhuis
bnoordhuis approved these changes Apr 11, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM sans comment.

src/inspector_io.cc Outdated
if (0 == uv_fs_realpath(&loop, &req, script_name_.c_str(), nullptr))
req.ptr = nullptr;
err = uv_fs_realpath(&loop, &req, script_name_.c_str(), nullptr);
if (0 == err) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to assign to err anymore, that would keep the diff smaller.

@eugeneo
Copy link
Contributor Author

eugeneo commented Apr 11, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/7335/

@eugeneo eugeneo closed this Apr 11, 2017
@eugeneo eugeneo deleted the signal-coverity branch April 11, 2017 23:04
@eugeneo eugeneo merged commit 42be835 into nodejs:master Apr 11, 2017
@eugeneo
Copy link
Contributor Author

eugeneo commented Apr 11, 2017

Landed as 42be835

@jasnell jasnell mentioned this pull request May 11, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu left review comments

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@eugeneo @bnoordhuis @jasnell @addaleax @TimothyGu @mscdex @nodejs-github-bot