You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by kansal <gi...@git.apache.org> on 2015/07/23 06:05:44 UTC
[GitHub] cloudstack pull request: Dereference NULL return value
GitHub user kansal opened a pull request:
https://github.com/apache/cloudstack/pull/618
Dereference NULL return value
No check on the null value of metadatafile. If null, the following operations failed. Added null check for the metadatafile.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kansal/cloudstack master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/618.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #618
----
commit aa0bd53162bd3f4450e5e7915e3b1f9e898fe55d
Author: Kshitij Kansal <ka...@kshitijs-macbook-pro.local>
Date: 2015-07-23T15:55:17Z
Dereference NULL return value
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/618#discussion_r35524889
--- Diff: plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java ---
@@ -119,12 +123,12 @@
@Override
public boolean start() {
if (isSAMLPluginEnabled()) {
- setup();
s_logger.info("SAML auth plugin loaded");
+ return setup();
} else {
--- End diff --
@bhaisaab I feel we can go with the if-else style because it anyhow is doing the same thing plus it give better code readability and logging. Your take?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/618#discussion_r35291257
--- Diff: plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java ---
@@ -368,8 +368,14 @@ private boolean setup() {
_idpMetaDataProvider = new HTTPMetadataProvider(_timer, client, idpMetaDataUrl);
} else {
File metadataFile = PropertiesUtil.findConfigFile(idpMetaDataUrl);
- s_logger.debug("Provided Metadata is not a URL, trying to read metadata file from local path: " + metadataFile.getAbsolutePath());
- _idpMetaDataProvider = new FilesystemMetadataProvider(_timer, metadataFile);
+ if (metadataFile == null) {
+ s_logger.error("Metadata file returned null");
--- End diff --
Can you also add the file path its trying to fing (idpMetaDataUrl) to the error message?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/618#discussion_r35521408
--- Diff: plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java ---
@@ -358,7 +363,8 @@ private boolean setup() {
}
_timer = new Timer();
final HttpClient client = new HttpClient();
- final String idpMetaDataUrl = SAMLIdentityProviderMetadataURL.value();
+ //final String idpMetaDataUrl = SAMLIdentityProviderMetadataURL.value();
--- End diff --
Remove commented deadcode, or simply keep final String idpMetaDataUrl = SAMLIdentityProviderMetadataURL.value(); as not many consumers of the new getSAMLIdentityProviderMetadataURL() method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on the pull request:
https://github.com/apache/cloudstack/pull/618#issuecomment-125106632
@DaanHoogland Added the test file. Checking only the false cases. @wilderrodrigues Got away with the IF statements and replaced with the api @karuturi suggested. Please review the changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by kansal <gi...@git.apache.org>.
Github user kansal closed the pull request at:
https://github.com/apache/cloudstack/pull/618
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/618#discussion_r35524999
--- Diff: plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java ---
@@ -358,7 +363,8 @@ private boolean setup() {
}
_timer = new Timer();
final HttpClient client = new HttpClient();
- final String idpMetaDataUrl = SAMLIdentityProviderMetadataURL.value();
+ //final String idpMetaDataUrl = SAMLIdentityProviderMetadataURL.value();
--- End diff --
@bhaisaab Sure. Removing the comment. The function call is needed for mocking at the time of unit testing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on the pull request:
https://github.com/apache/cloudstack/pull/618#issuecomment-125552998
Closing this pull request. Will open one for each issue.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by karuturi <gi...@git.apache.org>.
Github user karuturi commented on the pull request:
https://github.com/apache/cloudstack/pull/618#issuecomment-124330473
instead of a notnull check everywhere, can you use a Non Nulllable ArrayList for tags or use CollectionUtils.addIgnoreNull() api?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/618#issuecomment-125154356
@kansal Please see my comments. While it's okay, please squash the commits into two separate commits; one targeting the saml related stuff and second one targeting the other component, so as to allow us to port the commits to other branches.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/618#issuecomment-124034602
thanks @kansal
Can you add some test cases (for null values found for instance)?
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/618#discussion_r35521326
--- Diff: plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java ---
@@ -119,12 +123,12 @@
@Override
public boolean start() {
if (isSAMLPluginEnabled()) {
- setup();
s_logger.info("SAML auth plugin loaded");
+ return setup();
} else {
--- End diff --
we can remove else {} here just keep;
if (condition){
return stuff();
}
return otherstuff();
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on the pull request:
https://github.com/apache/cloudstack/pull/618#issuecomment-125150991
LGTM :+1:
Thanks for the improvements!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by kishankavala <gi...@git.apache.org>.
Github user kishankavala commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/618#discussion_r35290636
--- Diff: plugins/user-authenticators/saml2/src/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java ---
@@ -368,8 +368,14 @@ private boolean setup() {
_idpMetaDataProvider = new HTTPMetadataProvider(_timer, client, idpMetaDataUrl);
} else {
File metadataFile = PropertiesUtil.findConfigFile(idpMetaDataUrl);
- s_logger.debug("Provided Metadata is not a URL, trying to read metadata file from local path: " + metadataFile.getAbsolutePath());
- _idpMetaDataProvider = new FilesystemMetadataProvider(_timer, metadataFile);
+ if (metadataFile == null) {
+ s_logger.error("Metadata file returned null");
--- End diff --
Can you please update the error message: "Provided Metadata is not a URL, Unable to locate metadata file from local path"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by kishankavala <gi...@git.apache.org>.
Github user kishankavala commented on the pull request:
https://github.com/apache/cloudstack/pull/618#issuecomment-125136388
LGTM. @bhaisaab can you also take a look at the changes?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Dereference NULL return value
Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on the pull request:
https://github.com/apache/cloudstack/pull/618#issuecomment-124034693
@kishankavala: The message has been changed.
@karuturi: Added the path. Please check
I have also made another change with respect to solving this problem of dereferencing of null pointer. Kindly review the changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---