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

STORM-3470: fix null dereference in SimpleSaslServer authentication #3088

Merged
merged 1 commit into from
Jul 19, 2019
Merged

STORM-3470: fix null dereference in SimpleSaslServer authentication #3088

merged 1 commit into from
Jul 19, 2019

Conversation

nescohen
Copy link

Fixes a possible null dereference in the case that nid is null on line 183. Switch to using a more flexible method of comparison (Objects.compare()) which will correctly compare one null on one non-null or two null values.

@srdo
Copy link
Contributor

srdo commented Jul 18, 2019

Thanks for the contribution @nescohen. I think we should create an issue to track this, as it's fixing a bug. Otherwise it won't show up in release notes, and it won't be obvious which Storm releases have this issue.

@nescohen
Copy link
Author

@srdo, sounds good. I can create a jira issue and update the pr accordingly. Thanks!

@Ethanlm
Copy link
Contributor

Ethanlm commented Jul 18, 2019

Nice catch. Thanks for contribution

@nescohen nescohen changed the title MINOR: fix null dereference in SimpleSaslServer authentication STORM-3470: fix null dereference in SimpleSaslServer authentication Jul 19, 2019
@nescohen
Copy link
Author

@srdo, Hey just update the PR with the jira issue number :)

@nescohen
Copy link
Author

nescohen commented Jul 19, 2019

Nice catch. Thanks for contribution

Yeah, No problem :) This issue was actually found by Muse, an exciting upcoming static analysis platform focusing on finding deep, hard-to-find bugs so that developers can spend their time doing the fun part of writing new code. It will be free for life for open source projects. Would you guys be interested in turning it on for Storm? I would be happy to get you an activation code.

@Ethanlm
Copy link
Contributor

Ethanlm commented Jul 19, 2019

Nice catch. Thanks for contribution

Yeah, No problem :) This issue was actually found by Muse, an exciting upcoming static analysis platform focusing on finding deep, hard-to-find bugs so that developers can spend their time doing the fun part of writing new code. It will be free for life for open source projects. Would you guys be interested in turning it on for Storm? I would be happy to get you an activation code.

I am not familiar with this tool. It might worth to discuss it in the dev mailing list for this.

@Ethanlm
Copy link
Contributor

Ethanlm commented Jul 19, 2019

Please also update your commit message to include the jira number. I can then merge it. Thanks

@nescohen
Copy link
Author

Please also update your commit message to include the jira number. I can then merge it. Thanks

Done. No Problem!

@nescohen
Copy link
Author

Nice catch. Thanks for contribution

Yeah, No problem :) This issue was actually found by Muse, an exciting upcoming static analysis platform focusing on finding deep, hard-to-find bugs so that developers can spend their time doing the fun part of writing new code. It will be free for life for open source projects. Would you guys be interested in turning it on for Storm? I would be happy to get you an activation code.

I am not familiar with this tool. It might worth to discuss it in the dev mailing list for this.

For sure, I will reach out there.

Just to give you some context, I don't know if you are familiar with open source static analysis tools Infer from facebook and error-prone from Google. They can provide some really helpful results, but the problem is that they were created for the most part by researchers, and they tend to be less than convenient to get working. At MuseDev, we have been working to create a platform that can take a lot of this burden off of the end user, as well as providing results from a suite of tools and aggregating them together.

@Ethanlm Ethanlm merged commit 2576e38 into apache:master Jul 19, 2019
@Ethanlm
Copy link
Contributor

Ethanlm commented Jul 19, 2019

Cool Thanks for the information. I will definitely take a look when I get a chance.

@nescohen
Copy link
Author

Cool Thanks for the information. I will definitely take a look when I get a chance.

@Ethanlm Awesome. Feel free to reach to me if you have any questions!

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.

3 participants