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.

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: fix some recently introduced coverity issues #47240

Closed
wants to merge 3 commits into from

Conversation

mhdawson
Copy link
Member

No description provided.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 23, 2023
@mhdawson
Copy link
Member Author

mhdawson commented Mar 23, 2023

Coverity reports

src/dataqueue/queue.cc
public:
 788  static std::unique_ptr<FdEntry> Create(Environment* env, Local<Value> path) {
 789    // We're only going to create the FdEntry if the file exists.
    	1. var_decl: Declaring variable req without initializer.
 790    uv_fs_t req;
 791    auto cleanup = OnScopeLeave([&] { uv_fs_req_cleanup(&req); });
 792
 793    auto buf = std::make_shared<BufferValue>(env->isolate(), path);
    	2. Condition uv_fs_stat(NULL, &req, buf->out(), NULL) < 0, taking false branch.
 794    if (uv_fs_stat(nullptr, &req, buf->out(), nullptr) < 0) return nullptr;
 795
    	
CID 310020 (#1 of 1): Uninitialized scalar variable (UNINIT)
3. uninit_use_in_call: Using uninitialized value req.statbuf when calling make_unique.
 796    return std::make_unique<FdEntry>(
 797        env, std::move(buf), req.statbuf, 0, req.statbuf.st_size);
 798  }

static bool CheckModified(FdEntry* entry, int fd) {
    	1. var_decl: Declaring variable req without initializer.
 852    uv_fs_t req;
 853    auto cleanup = OnScopeLeave([&] { uv_fs_req_cleanup(&req); });
 854    // TODO(jasnell): Note the use of a sync fs call here is a bit unfortunate.
 855    // Doing this asynchronously creates a bit of a race condition tho, a file
 856    // could be unmodified when we call the operation but then by the time the
 857    // async callback is triggered to give us that answer the file is modified.
 858    // While such silliness is still possible here, the sync call at least makes
 859    // it less likely to hit the race.
    	2. Condition uv_fs_fstat(NULL, &req, fd, NULL) < 0, taking false branch.
 860    if (uv_fs_fstat(nullptr, &req, fd, nullptr) < 0) return true;
    	3. uninit_use_in_call: Using uninitialized value req.statbuf.st_size when calling is_modified. [[show details](https://scan9.scan.coverity.com/eventId=9011842-3&modelId=9011842-0&fileInstanceId=111313726&filePath=%2Fsrc%2Fdataqueue%2Fqueue.cc&fileStart=846&fileEnd=849)]
    	
CID 310018 (#1-2 of 2): Uninitialized scalar variable (UNINIT)
4. uninit_use_in_call: Using uninitialized value req.statbuf.st_mtim.tv_nsec when calling is_modified. [[show details](https://scan9.scan.coverity.com/eventId=9011842-6&modelId=9011842-1&fileInstanceId=111313726&filePath=%2Fsrc%2Fdataqueue%2Fqueue.cc&fileStart=846&fileEnd=849)]
 861    return entry->is_modified(req.statbuf);
 862  }

test/embedding/embedtest.cc

    } else {
   	5. var_decl: Declaring variable req without initializer.
 78      uv_fs_t req;
 79      int statret = uv_fs_stat(nullptr, &req, filename, nullptr);
   	6. Condition statret == 0, taking true branch.
 80      assert(statret == 0);
   	
CID 307777 (#1 of 1): Uninitialized scalar variable (UNINIT)
7. uninit_use: Using uninitialized value req.statbuf.st_size.
 81      size_t filesize = req.statbuf.st_size;
 82      uv_fs_req_cleanup(&req);
 83
 84      std::vector<char> vec(filesize);
 85      size_t read = fread(vec.data(), filesize, 1, fp);
 86      assert(read == 1);
 87      snapshot = node::EmbedderSnapshotData::FromBlob(vec);
 88    }

@mhdawson
Copy link
Member Author

mhdawson commented Mar 23, 2023

Not sure why it would have failed for coverage and not for my local make test linux run, will have to look at it tomorrow.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@richardlau
Copy link
Member

Not sure why it would have failed for coverage and not for my local make test linux run, will have to look at it tomorrow.

My guess would be either compiler level and/or the --error-on-warn configure flag.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@mhdawson
Copy link
Member Author

Updated to initialize in another way, lets see if the compilers are happy with it across platforms. Also compiled ok locally.

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 24, 2023
@mhdawson
Copy link
Member Author

Requesting ci to check across all platforms

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 24, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mhdawson
Copy link
Member Author

Landed in fe449a2

@mhdawson mhdawson closed this Mar 28, 2023
mhdawson added a commit that referenced this pull request Mar 28, 2023
Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #47240
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #47240
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #47240
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
Signed-off-by: Michael Dawson <mdawson@devrus.com>

PR-URL: #47240
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@danielleadams danielleadams added the dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants