Skip to content

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