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

Channel notify method error handling #1783

Closed

Conversation

deweerdt
Copy link
Member

@deweerdt deweerdt commented Jun 6, 2018

No description provided.

coverity finding CID 1469813
@deweerdt deweerdt force-pushed the channel_notify_method-error-handling branch from deb3782 to 91ff99e Compare June 6, 2018 20:56
@kazuho
Copy link
Member

kazuho commented Jun 12, 2018

Thank you for the PR.

Is this PR based on your observation that we have a code path that reaches this function without the context variable being non-NULL?

@kazuho kazuho requested a review from i110 June 12, 2018 06:06
@deweerdt
Copy link
Member Author

deweerdt commented Jun 12, 2018

@kazuho this is the same observation by coverity as for EVP_EncryptInit_ex most of the other cases in the code base check for the error return. If it's impossible in this case -- i don't know about mruby to understand if this can be an issue -- we should close this PR.

@kazuho
Copy link
Member

kazuho commented Jun 13, 2018

Thank you for the clarification. I'll defer the decision to @i110. I believe that we have other parts of code that depend on mrb_data_check_get_ptr, and that we should have a consistent coding pattern assuming that the type of the object being passed to the caller of the get_ptr function is known to be of the correct type.

@i110
Copy link
Contributor

i110 commented Jun 18, 2018

I guess there's no chance to have this ctx be NULL, but as you know Ruby lets us various magics, so I think we should handle errors in any cases. Then, here we should rather use mrb_data_get_ptr that does that error checks and raises an TypeError in irregular cases. At least, we must not return the error object (it has no effect here)

@i110
Copy link
Contributor

i110 commented Jun 18, 2018

I filed #1794, so close this. @deweerdt Thank you for finding the issue!

@i110 i110 closed this Jun 18, 2018
Copy link
Contributor

@i110 i110 left a comment

Choose a reason for hiding this comment

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

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants