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

wasi: address coverity warning #49866

Closed
wants to merge 1 commit into from
Closed

Conversation

mhdawson
Copy link
Member

  • add check for case when trying to provide a better Exception fails
  • the code was modified to avoid a CHECK_EQ in all cases in wasi: throw on failed uvwasi_init() #31076, however, I believe that if we fail to create the exeption to throw instead of simply returning using a CHECK makes more sense. I think it should also address the coverity warning about not initializing in the constructor.

- add check for case when trying to provide
  a better Exception fails
- the code was modified to avoid a CHECK_EQ in all
  cases in nodejs#31076,
  however, I believe that if we fail to create the exeption
  to throw instead of simply returning using a CHECK makes
  more sense. I think it should also address the coverity
  warning about not initializing in the constructor.

Signed-off-by: Michael Dawson <midawson@redhat.com>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/wasi

@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. wasi Issues and PRs related to the WebAssembly System Interface. labels Sep 25, 2023
@mhdawson
Copy link
Member Author

Coverity warning:

 84  if (err != UVWASI_ESUCCESS) {
 85    Local<Value> exception;
   	2. Condition !v8::MaybeLocal<v8::Value>(node::wasi::WASIException(env->context(), err, "uvwasi_init")).ToLocal(&exception), taking true branch.
 86    if (!WASIException(env->context(), err, "uvwasi_init").ToLocal(&exception))
   	4. uninit_member: Non-static class member uvw_.fds is not initialized in this constructor nor in any functions that it calls.
   	6. uninit_member: Non-static class member uvw_.argc is not initialized in this constructor nor in any functions that it calls.
   	8. uninit_member: Non-static class member uvw_.argv is not initialized in this constructor nor in any functions that it calls.
   	10. uninit_member: Non-static class member uvw_.argv_buf is not initialized in this constructor nor in any functions that it calls.
   	12. uninit_member: Non-static class member uvw_.argv_buf_size is not initialized in this constructor nor in any functions that it calls.
   	14. uninit_member: Non-static class member uvw_.envc is not initialized in this constructor nor in any functions that it calls.
   	16. uninit_member: Non-static class member uvw_.env is not initialized in this constructor nor in any functions that it calls.
   	18. uninit_member: Non-static class member uvw_.env_buf is not initialized in this constructor nor in any functions that it calls.
   	20. uninit_member: Non-static class member uvw_.env_buf_size is not initialized in this constructor nor in any functions that it calls.
   	
CID 276376 (#2 of 2): Uninitialized pointer field (UNINIT_CTOR)
22. uninit_member: Non-static class member uvw_.allocator is not initialized in this constructor nor in any functions that it calls.
 87      return;
 88
 89    env->isolate()->ThrowException(exception);

@mhdawson mhdawson added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Sep 28, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 28, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@devsnek
Copy link
Member

devsnek commented Sep 29, 2023

is this condition reachable when the isolate is terminated?

@mhdawson
Copy link
Member Author

mhdawson commented Oct 4, 2023

Since there is a return code from the function around which the CHECK as been added I believe it should be reachable.

mhdawson added a commit that referenced this pull request Oct 12, 2023
- add check for case when trying to provide
  a better Exception fails
- the code was modified to avoid a CHECK_EQ in all
  cases in #31076,
  however, I believe that if we fail to create the exeption
  to throw instead of simply returning using a CHECK makes
  more sense. I think it should also address the coverity
  warning about not initializing in the constructor.

Signed-off-by: Michael Dawson <midawson@redhat.com>

PR-URL: #49866
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mhdawson
Copy link
Member Author

Landed in c1a3a98

@mhdawson mhdawson closed this Oct 12, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
- add check for case when trying to provide
  a better Exception fails
- the code was modified to avoid a CHECK_EQ in all
  cases in nodejs#31076,
  however, I believe that if we fail to create the exeption
  to throw instead of simply returning using a CHECK makes
  more sense. I think it should also address the coverity
  warning about not initializing in the constructor.

Signed-off-by: Michael Dawson <midawson@redhat.com>

PR-URL: nodejs#49866
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Nov 11, 2023
- add check for case when trying to provide
  a better Exception fails
- the code was modified to avoid a CHECK_EQ in all
  cases in #31076,
  however, I believe that if we fail to create the exeption
  to throw instead of simply returning using a CHECK makes
  more sense. I think it should also address the coverity
  warning about not initializing in the constructor.

Signed-off-by: Michael Dawson <midawson@redhat.com>

PR-URL: #49866
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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++. needs-ci PRs that need a full CI run. wasi Issues and PRs related to the WebAssembly System Interface.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants