You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by DaanHoogland <gi...@git.apache.org> on 2015/11/10 16:22:00 UTC
[GitHub] cloudstack pull request: Cwe 190
GitHub user DaanHoogland opened a pull request:
https://github.com/apache/cloudstack/pull/1057
Cwe 190
coverity warnings of this type adressed. Some where dismissed and maybe with reason but it seemed possible to remove them and hence obligatory ;p
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaanHoogland/cloudstack CWE-190
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1057.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 #1057
----
commit 944e6eabdff9b01f8980277fbe5dda5228fa94ad
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-11-10T13:00:22Z
CID-1175714 casts before bit shift
commit a9e122c77227363f3f27dbea7d13b4110c07a96f
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-11-10T13:16:49Z
CID-1116485: cast cidr during bit shifting
and simple test included
commit 17849c3b4b66a96c32d7ed8fb713f763ef19ee39
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-11-10T13:40:24Z
CID-1116484 cast to long and use long as cidrsize type
and simpel test
commit 1559e351d2a56a33ee3d844ce4964e5c4420e439
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-11-10T13:54:52Z
CID-1116483 cidr to netmask bitshifts guarded with casts
and simple test
commit f04b68c99c2d112b83617be7d51ac351a1882254
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-11-10T15:18:52Z
CID-1116482 cidrToLong cleanup of bitshift problem
and some trivial 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: Cwe 190
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-155517568
@DaanHoogland, pretty good job, you created the test cases and removed the return null cases.
I just have one question, why you use a cast for the octal value “0xffffffff” to long? Simple using 0xffffffff is not enough?
---
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: Cwe 190
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-159127988
@remibergsma, could you send me the stack trace of the test that failed?
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1057#discussion_r44515734
--- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
@@ -869,31 +878,44 @@ public static boolean isNetworkAWithinNetworkB(final String cidrA, final String
public static Long[] cidrToLong(final String cidr) {
if (cidr == null || cidr.isEmpty()) {
- return null;
+ throw new CloudRuntimeException("empty cidr can not be converted to longs");
}
final String[] cidrPair = cidr.split("\\/");
if (cidrPair.length != 2) {
- return null;
+ throw new CloudRuntimeException("cidr is not formatted correctly: "+ cidr);
}
final String cidrAddress = cidrPair[0];
final String cidrSize = cidrPair[1];
if (!isValidIp(cidrAddress)) {
- return null;
- }
- int cidrSizeNum = -1;
-
- try {
- cidrSizeNum = Integer.parseInt(cidrSize);
- } catch (final Exception e) {
- return null;
+ throw new CloudRuntimeException("cidr is not bvalid in ip space" + cidr);
}
- final long numericNetmask = 0xffffffff >> MAX_CIDR - cidrSizeNum << MAX_CIDR - cidrSizeNum;
+ long cidrSizeNum = getCidrSizeFromString(cidrSize);
+ final long numericNetmask = netMaskFromCidr(cidrSizeNum);
final long ipAddr = ip2Long(cidrAddress);
final Long[] cidrlong = {ipAddr & numericNetmask, (long)cidrSizeNum};
return cidrlong;
}
+ /**
+ * @param cidrSize
+ * @return
+ * @throws CloudRuntimeException
+ */
+ static long getCidrSizeFromString(final String cidrSize) throws CloudRuntimeException {
+ long cidrSizeNum = -1;
+
+ try {
+ cidrSizeNum = Integer.parseInt(cidrSize);
+ } catch (final NumberFormatException e) {
+ throw new CloudRuntimeException("cidrsize is not a valid int: " + cidrSize, e);
+ }
+ if(cidrSizeNum > 32 || cidrSizeNum < 0) {// assuming IPv4
+ throw new CloudRuntimeException("cidr size out of range: " + cidrSizeNum);
+ }
+ return cidrSizeNum;
+ }
+
--- End diff --
for testing purpose
---
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: Cwe 190
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-155595924
Got it @DaanHoogland.
What about using () to surround the “(long) 0xffffffff” in lines 885, 925 and 931 as you did in 779.
---
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: Cwe 190
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-162287862
LGTM based on these tests:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a tags=advanced,required_hardware=true \
component/test_vpc_redundant.py \
component/test_routers_iptables_default_policy.py \
component/test_routers_network_ops.py \
component/test_vpc_router_nics.py \
smoke/test_loadbalance.py \
smoke/test_internal_lb.py \
smoke/test_ssvm.py \
smoke/test_network.py
```
Result:
```
Create a redundant VPC with two networks with two VMs in each network ... === TestName: test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | Status : SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network and check default routes ... === TestName: test_02_redundant_VPC_default_routes | Status : SUCCESS ===
ok
Create a redundant VPC with two networks with two VMs in each network ... === TestName: test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | Status : SUCCESS ===
ok
Test iptables default INPUT/FORWARD policy on RouterVM ... === TestName: test_02_routervm_iptables_policies | Status : SUCCESS ===
ok
Test iptables default INPUT/FORWARD policies on VPC router ... === TestName: test_01_single_VPC_iptables_policies | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: test_01_isolate_network_FW_PF_default_routes_egress_true | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: test_02_isolate_network_FW_PF_default_routes_egress_false | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true | Status : SUCCESS ===
ok
Test redundant router internals ... === TestName: test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false | Status : SUCCESS ===
ok
Create a VPC with two networks with one VM in each network and test nics after destroy ... === TestName: test_01_VPC_nics_after_destroy | Status : SUCCESS ===
ok
Create a VPC with two networks with one VM in each network and test default routes ... === TestName: test_02_VPC_default_routes | Status : SUCCESS ===
ok
Check the password file in the Router VM ... === TestName: test_isolate_network_password_server | Status : SUCCESS ===
ok
Check that the /etc/dhcphosts.txt doesn't contain duplicate IPs ... === TestName: test_router_dhcphosts | Status : SUCCESS ===
ok
Test to create Load balancing rule with source NAT ... === TestName: test_01_create_lb_rule_src_nat | Status : SUCCESS ===
ok
Test to create Load balancing rule with non source NAT ... === TestName: test_02_create_lb_rule_non_nat | Status : SUCCESS ===
ok
Test for assign & removing load balancing rule ... === TestName: test_assign_and_removal_lb | Status : SUCCESS ===
ok
Test to verify access to loadbalancer haproxy admin stats page ... === TestName: test02_internallb_haproxy_stats_on_all_interfaces | Status : SUCCESS ===
ok
Test create, assign, remove of an Internal LB with roundrobin http traffic to 3 vm's ... === TestName: test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 | Status : SUCCESS ===
ok
Test SSVM Internals ... === TestName: test_03_ssvm_internals | Status : SUCCESS ===
ok
Test CPVM Internals ... === TestName: test_04_cpvm_internals | Status : SUCCESS ===
ok
Test stop SSVM ... === TestName: test_05_stop_ssvm | Status : SUCCESS ===
ok
Test stop CPVM ... === TestName: test_06_stop_cpvm | Status : SUCCESS ===
ok
Test reboot SSVM ... === TestName: test_07_reboot_ssvm | Status : SUCCESS ===
ok
Test reboot CPVM ... === TestName: test_08_reboot_cpvm | Status : SUCCESS ===
ok
Test destroy SSVM ... === TestName: test_09_destroy_ssvm | Status : SUCCESS ===
ok
Test destroy CPVM ... === TestName: test_10_destroy_cpvm | Status : SUCCESS ===
ok
Test Remote Access VPN in VPC ... === TestName: test_vpc_remote_access_vpn | Status : SUCCESS ===
ok
Test VPN in VPC ... === TestName: test_vpc_site2site_vpn | Status : SUCCESS ===
ok
Test for port forwarding on source NAT ... === TestName: test_01_port_fwd_on_src_nat | Status : SUCCESS ===
ok
Test for port forwarding on non source NAT ... === TestName: test_02_port_fwd_on_non_src_nat | Status : SUCCESS ===
ok
Test for reboot router ... === TestName: test_reboot_router | Status : SUCCESS ===
ok
Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_1_static_nat_rule | Status : SUCCESS ===
ok
Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_2_nat_rule | Status : SUCCESS ===
ok
Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_3_Load_Balancer_Rule | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 34 tests in 17179.953s
OK
```
And:
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a tags=advanced,required_hardware=false \
smoke/test_routers.py \
smoke/test_network_acl.py \
smoke/test_privategw_acl.py \
smoke/test_reset_vm_on_reboot.py \
smoke/test_vm_life_cycle.py \
smoke/test_vpc_vpn.py \
smoke/test_service_offerings.py \
component/test_vpc_offerings.py \
component/test_vpc_routers.py
```
Result:
```
Test router internal advanced zone ... === TestName: test_02_router_internal_adv | Status : SUCCESS ===
ok
Test restart network ... === TestName: test_03_restart_network_cleanup | Status : SUCCESS ===
ok
Test router basic setup ... === TestName: test_05_router_basic | Status : SUCCESS ===
ok
Test router advanced setup ... === TestName: test_06_router_advanced | Status : SUCCESS ===
ok
Test stop router ... === TestName: test_07_stop_router | Status : SUCCESS ===
ok
Test start router ... === TestName: test_08_start_router | Status : SUCCESS ===
ok
Test reboot router ... === TestName: test_09_reboot_router | Status : SUCCESS ===
ok
Test reset virtual machine on reboot ... === TestName: test_01_reset_vm_on_reboot | Status : SUCCESS ===
ok
Test advanced zone virtual router ... === TestName: test_advZoneVirtualRouter | Status : SUCCESS ===
ok
Test Deploy Virtual Machine ... === TestName: test_deploy_vm | Status : SUCCESS ===
ok
Test Multiple Deploy Virtual Machine ... === TestName: test_deploy_vm_multiple | Status : SUCCESS ===
ok
Test Stop Virtual Machine ... === TestName: test_01_stop_vm | Status : SUCCESS ===
ok
Test Start Virtual Machine ... === TestName: test_02_start_vm | Status : SUCCESS ===
ok
Test Reboot Virtual Machine ... === TestName: test_03_reboot_vm | Status : SUCCESS ===
ok
Test destroy Virtual Machine ... === TestName: test_06_destroy_vm | Status : SUCCESS ===
ok
Test recover Virtual Machine ... === TestName: test_07_restore_vm | Status : SUCCESS ===
ok
Test migrate VM ... === TestName: test_08_migrate_vm | Status : SUCCESS ===
ok
Test destroy(expunge) Virtual Machine ... === TestName: test_09_expunge_vm | Status : SUCCESS ===
ok
Test to create service offering ... === TestName: test_01_create_service_offering | Status : SUCCESS ===
ok
Test to update existing service offering ... === TestName: test_02_edit_service_offering | Status : SUCCESS ===
ok
Test to delete service offering ... === TestName: test_03_delete_service_offering | Status : SUCCESS ===
ok
Test for delete account ... === TestName: test_delete_account | Status : SUCCESS ===
ok
Test for Associate/Disassociate public IP address for admin account ... === TestName: test_public_ip_admin_account | Status : SUCCESS ===
ok
Test for Associate/Disassociate public IP address for user account ... === TestName: test_public_ip_user_account | Status : SUCCESS ===
ok
Test for release public IP address ... === TestName: test_releaseIP | Status : SUCCESS ===
ok
Test create VPC offering ... === TestName: test_01_create_vpc_offering | Status : SUCCESS ===
ok
Test VPC offering without load balancing service ... === TestName: test_03_vpc_off_without_lb | Status : SUCCESS ===
ok
Test VPC offering without static NAT service ... === TestName: test_04_vpc_off_without_static_nat | Status : SUCCESS ===
ok
Test VPC offering without port forwarding service ... === TestName: test_05_vpc_off_without_pf | Status : SUCCESS ===
ok
Test VPC offering with invalid services ... === TestName: test_06_vpc_off_invalid_services | Status : SUCCESS ===
ok
Test update VPC offering ... === TestName: test_07_update_vpc_off | Status : SUCCESS ===
ok
Test list VPC offering ... === TestName: test_08_list_vpc_off | Status : SUCCESS ===
ok
test_09_create_redundant_vpc_offering (integration.component.test_vpc_offerings.TestVPCOffering) ... === TestName: test_09_create_redundant_vpc_offering | Status : SUCCESS ===
ok
Test start/stop of router after addition of one guest network ... === TestName: test_01_start_stop_router_after_addition_of_one_guest_network | Status : SUCCESS ===
ok
Test reboot of router after addition of one guest network ... === TestName: test_02_reboot_router_after_addition_of_one_guest_network | Status : SUCCESS ===
ok
Test to change service offering of router after addition of one guest network ... === TestName: test_04_chg_srv_off_router_after_addition_of_one_guest_network | Status : SUCCESS ===
ok
Test destroy of router after addition of one guest network ... === TestName: test_05_destroy_router_after_addition_of_one_guest_network | Status : SUCCESS ===
ok
Test to stop and start router after creation of VPC ... === TestName: test_01_stop_start_router_after_creating_vpc | Status : SUCCESS ===
ok
Test to reboot the router after creating a VPC ... === TestName: test_02_reboot_router_after_creating_vpc | Status : SUCCESS ===
ok
Tests to change service offering of the Router after ... === TestName: test_04_change_service_offerring_vpc | Status : SUCCESS ===
ok
Test to destroy the router after creating a VPC ... === TestName: test_05_destroy_router_after_creating_vpc | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 41 tests in 8373.911s
OK
```
I didn't test the actual feature. This just shows it didn't break anything else.
---
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: Cwe 190
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-155710144
That is it.
Everything looks good to me now
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-158664606
@remibergsma for me test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL fails. That is not the one you meant is it? I also saw the failure you had.
---
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: Cwe 190
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-158613419
@DaanHoogland @rafaelweingartner Much better now, just one failed test to resolve:
Failed test: `TestVPCRouterOneNetwork`
```
Execute cmd: createstaticroute failed, due to: errorCode: 530, errorText:cidr is not formatted correctly: cloud
```
Details:
```
requests.packages.urllib3.connectionpool: DEBUG: "GET /client/api?apiKey=Mn6U73dZiNUEuY1GR7hwRNlwoPAiIeEbPnNZojmWoLNr31Y3b9SpBvuOP_xH_Pn7pm-BS1DuOrV8BffcLP35Uw&command=createStaticRoute&signature=QKOVZRSgas3u2sOUus5t8JncRbs%3D&gatewayid=0177e150-3dc2-4454-85be-f43b9b093b65&cidr=11.1.1.1%2F24&response=json HTTP/1.1" 530 133
test_09_create_redundant_vpc_offering (integration.component.test_vpc_offerings.TestVPCOffering): ERROR: Exception:['Traceback (most recent call last):\n', ' File "/usr/lib/python2.7/site-packages/marvin/cloudstackConnection.py", line 308, in __parseAndGetResponse\n response_cls)\n', ' File "/usr/lib/python2.7/site-packages/marvin/jsonHelper.py", line 150, in getResultObj\n raise cloudstackException.CloudstackAPIException(respname, errMsg)\n', 'CloudstackAPIException: Execute cmd: createstaticroute failed, due to: errorCode: 530, errorText:cidr is not formatted correctly: cloud\n']
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/marvin/cloudstackConnection.py", line 308, in __parseAndGetResponse
response_cls)
File "/usr/lib/python2.7/site-packages/marvin/jsonHelper.py", line 150, in getResultObj
raise cloudstackException.CloudstackAPIException(respname, errMsg)
CloudstackAPIException: Execute cmd: createstaticroute failed, due to: errorCode: 530, errorText:cidr is not formatted correctly: cloud
test_09_create_redundant_vpc_offering (integration.component.test_vpc_offerings.TestVPCOffering): ERROR: marvinRequest : CmdName: <marvin.cloudstackAPI.createStaticRoute.createStaticRouteCmd object at 0x2c13e90> Exception: ['Traceback (most recent call last):\n', ' File "/usr/lib/python2.7/site-packages/marvin/cloudstackConnection.py", line 374, in marvinRequest\n raise self.__lastError\n', 'CloudstackAPIException: Execute cmd: createstaticroute failed, due to: errorCode: 530, errorText:cidr is not formatted correctly: cloud\n']
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/marvin/cloudstackConnection.py", line 374, in marvinRequest
raise self.__lastError
CloudstackAPIException: Execute cmd: createstaticroute failed, due to: errorCode: 530, errorText:cidr is not formatted correctly: cloud
test_09_create_redundant_vpc_offering (integration.component.test_vpc_offerings.TestVPCOffering): CRITICAL: EXCEPTION: test_09_create_redundant_vpc_offering: ['Traceback (most recent call last):\n', ' File "/usr/lib/python2.7/site-packages/nose/suite.py", line 209, in run\n self.setUp()\n', ' File "/usr/lib/python2.7/site-packages/nose/suite.py", line 292, in setUp\n self.setupContext(ancestor)\n', ' File "/usr/lib/python2.7/site-packages/nose/suite.py", line 315, in setupContext\n try_run(context, names)\n', ' File "/usr/lib/python2.7/site-packages/nose/util.py", line 471, in try_run\n return func()\n', ' File "/data/jenkins-slave/workspace/acs-ci-build/901-run-marvin-tests/test/integration/component/test_vpc_routers.py", line 701, in setUpClass\n gatewayid=private_gateway.id\n', ' File "/usr/lib/python2.7/site-packages/marvin/lib/base.py", line 4262, in create\n return StaticRoute(apiclient.createStaticRoute(cmd).__dict__)\n', ' File "/usr/lib/python2.7
/site-packages/marvin/cloudstackAPI/cloudstackAPIClient.py", line 2277, in createStaticRoute\n response = self.connection.marvinRequest(command, response_type=response, method=method)\n', ' File "/usr/lib/python2.7/site-packages/marvin/cloudstackConnection.py", line 379, in marvinRequest\n raise e\n', 'CloudstackAPIException: Execute cmd: createstaticroute failed, due to: errorCode: 530, errorText:cidr is not formatted correctly: cloud\n']
```
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-159176971
[1057.network.results.txt](https://github.com/apache/cloudstack/files/42440/1057.network.results.txt)
[1057.vpc.results.txt](https://github.com/apache/cloudstack/files/42441/1057.vpc.results.txt)
Rafael, here are mine the first test in the first suite is one remaining.
---
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: Cwe 190
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-158625241
:-)
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-158620306
thanks @remibergsma ```cidr is not formatted correctly: cloud``` sound like a valid error message. not sure if it needs fixing in the test or in the code.
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-155710622
@rafaelweingartner yes of course. you caught me pants down ;)
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-158400754
to many unexplained failures for such a small refactor.
---
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: Cwe 190
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1057#discussion_r44524995
--- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
@@ -775,11 +776,19 @@ public static String getSubNet(final String ip, final String netmask) {
}
public static String getCidrSubNet(final String ip, final long cidrSize) {
- final long numericNetmask = 0xffffffff >> MAX_CIDR - cidrSize << MAX_CIDR - cidrSize;
+ final long numericNetmask = netMaskFromCidr(cidrSize);
final String netmask = NetUtils.long2Ip(numericNetmask);
return getSubNet(ip, netmask);
}
+ /**
+ * @param cidrSize
+ * @return
+ */
+ static long netMaskFromCidr(final long cidrSize) {
+ return ((long)0xffffffff) >> MAX_CIDR - cidrSize << MAX_CIDR - cidrSize;
+ }
+
--- End diff --
I think that it is fine letting the method with default modifier.
The point of a test case is that we can/should write tests for a single method. Therefore, if we want to test the method “getCidrSizeFromString”, we should write a test case for that specific method, and not for another one that uses it.
After we make sure that a method works fine, when we test another method (let’s say “cidrToLong”) that uses the first one already tested, we can use EasyMock/Mockito or another test suite to mock the method that we know works, this way we facilitate the writing of test cases.
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-155708663
@rafaelweingartner sure, they're not needed but I'll add them for clarity.
---
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: Cwe 190
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-158434570
Hi @DaanHoogland,
Do not give up now, I really liked your PR.
Those errors can be easily explained. You changed the behavior of com.cloud.utils.net.NetUtils.cidrToLong(String), instead of a null when receiving empty Cidrs, the method now throws an exception. That is why some error happened. To fix that is actually pretty easy, you just need to work on methods “isNetowrkASubsetOrSupersetOfNetworkB”, “isNetworkAWithinNetworkB” and “isNetworksOverlap”.
I have created a PR for your branch, so you can get those changes and reopen your PR.
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-158486604
@rafaelweingartner thanks for picking this up. it is not a matter of giving up but of priorities and I didn't want to keep it open knowing there was work to do that I wouldn't look into.
I'll have a look at your PR and run the integration suite against them.
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-155577386
@rafaelweingartner the cast was the reason for starting this. Coverity complained about a 32 bit shift on an integer (31 bit + sign)
---
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: Cwe 190
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-158499809
I understand why you closed the PR, but after a quick review, I thought that I could help you, so let’s see if now everything is ok
---
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: Cwe 190
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1057
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland closed the pull request at:
https://github.com/apache/cloudstack/pull/1057
---
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: Cwe 190
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1057#discussion_r44520446
--- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
@@ -869,31 +878,44 @@ public static boolean isNetworkAWithinNetworkB(final String cidrA, final String
public static Long[] cidrToLong(final String cidr) {
if (cidr == null || cidr.isEmpty()) {
- return null;
+ throw new CloudRuntimeException("empty cidr can not be converted to longs");
}
final String[] cidrPair = cidr.split("\\/");
if (cidrPair.length != 2) {
- return null;
+ throw new CloudRuntimeException("cidr is not formatted correctly: "+ cidr);
}
final String cidrAddress = cidrPair[0];
final String cidrSize = cidrPair[1];
if (!isValidIp(cidrAddress)) {
- return null;
- }
- int cidrSizeNum = -1;
-
- try {
- cidrSizeNum = Integer.parseInt(cidrSize);
- } catch (final Exception e) {
- return null;
+ throw new CloudRuntimeException("cidr is not bvalid in ip space" + cidr);
}
- final long numericNetmask = 0xffffffff >> MAX_CIDR - cidrSizeNum << MAX_CIDR - cidrSizeNum;
+ long cidrSizeNum = getCidrSizeFromString(cidrSize);
+ final long numericNetmask = netMaskFromCidr(cidrSizeNum);
final long ipAddr = ip2Long(cidrAddress);
final Long[] cidrlong = {ipAddr & numericNetmask, (long)cidrSizeNum};
return cidrlong;
}
+ /**
+ * @param cidrSize
+ * @return
+ * @throws CloudRuntimeException
+ */
+ static long getCidrSizeFromString(final String cidrSize) throws CloudRuntimeException {
+ long cidrSizeNum = -1;
+
+ try {
+ cidrSizeNum = Integer.parseInt(cidrSize);
+ } catch (final NumberFormatException e) {
+ throw new CloudRuntimeException("cidrsize is not a valid int: " + cidrSize, e);
+ }
+ if(cidrSizeNum > 32 || cidrSizeNum < 0) {// assuming IPv4
+ throw new CloudRuntimeException("cidr size out of range: " + cidrSizeNum);
+ }
+ return cidrSizeNum;
+ }
+
--- End diff --
I'm saying you don't need to. The test will have the same effect and encapsulation will be use properly. I know that Mockito could be better if we wouldn't have to kill encapsulation, but if there is a method that already can access the "private" method, then no need to open it.
Perhaps after 4.6 we can put some effort in place and move towards JMockit.
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1057#discussion_r44515725
--- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
@@ -775,11 +776,19 @@ public static String getSubNet(final String ip, final String netmask) {
}
public static String getCidrSubNet(final String ip, final long cidrSize) {
- final long numericNetmask = 0xffffffff >> MAX_CIDR - cidrSize << MAX_CIDR - cidrSize;
+ final long numericNetmask = netMaskFromCidr(cidrSize);
final String netmask = NetUtils.long2Ip(numericNetmask);
return getSubNet(ip, netmask);
}
+ /**
+ * @param cidrSize
+ * @return
+ */
+ static long netMaskFromCidr(final long cidrSize) {
+ return ((long)0xffffffff) >> MAX_CIDR - cidrSize << MAX_CIDR - cidrSize;
+ }
+
--- End diff --
for testing purpose
---
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: Cwe 190
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1057#discussion_r44519815
--- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
@@ -869,31 +878,44 @@ public static boolean isNetworkAWithinNetworkB(final String cidrA, final String
public static Long[] cidrToLong(final String cidr) {
if (cidr == null || cidr.isEmpty()) {
- return null;
+ throw new CloudRuntimeException("empty cidr can not be converted to longs");
}
final String[] cidrPair = cidr.split("\\/");
if (cidrPair.length != 2) {
- return null;
+ throw new CloudRuntimeException("cidr is not formatted correctly: "+ cidr);
}
final String cidrAddress = cidrPair[0];
final String cidrSize = cidrPair[1];
if (!isValidIp(cidrAddress)) {
- return null;
- }
- int cidrSizeNum = -1;
-
- try {
- cidrSizeNum = Integer.parseInt(cidrSize);
- } catch (final Exception e) {
- return null;
+ throw new CloudRuntimeException("cidr is not bvalid in ip space" + cidr);
}
- final long numericNetmask = 0xffffffff >> MAX_CIDR - cidrSizeNum << MAX_CIDR - cidrSizeNum;
+ long cidrSizeNum = getCidrSizeFromString(cidrSize);
+ final long numericNetmask = netMaskFromCidr(cidrSizeNum);
final long ipAddr = ip2Long(cidrAddress);
final Long[] cidrlong = {ipAddr & numericNetmask, (long)cidrSizeNum};
return cidrlong;
}
+ /**
+ * @param cidrSize
+ * @return
+ * @throws CloudRuntimeException
+ */
+ static long getCidrSizeFromString(final String cidrSize) throws CloudRuntimeException {
+ long cidrSizeNum = -1;
+
+ try {
+ cidrSizeNum = Integer.parseInt(cidrSize);
+ } catch (final NumberFormatException e) {
+ throw new CloudRuntimeException("cidrsize is not a valid int: " + cidrSize, e);
+ }
+ if(cidrSizeNum > 32 || cidrSizeNum < 0) {// assuming IPv4
+ throw new CloudRuntimeException("cidr size out of range: " + cidrSizeNum);
+ }
+ return cidrSizeNum;
+ }
+
--- End diff --
But those methods are also used internally, right? I mean, if you call getCidrSubNet() it will call the other static method. So, you can make them private and manipulate the input via the getCidrSubNet() 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: Cwe 190
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-155710538
I was just re-checking the code, what about creating a test case for "netMaskFromCidr" 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: Cwe 190
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-158314116
FYI: @DaanHoogland My tests show 29 test failures. I will retest to be sure, but please hold until this is done.
---
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: Cwe 190
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1057#discussion_r44515608
--- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
@@ -869,31 +878,44 @@ public static boolean isNetworkAWithinNetworkB(final String cidrA, final String
public static Long[] cidrToLong(final String cidr) {
if (cidr == null || cidr.isEmpty()) {
- return null;
+ throw new CloudRuntimeException("empty cidr can not be converted to longs");
}
final String[] cidrPair = cidr.split("\\/");
if (cidrPair.length != 2) {
- return null;
+ throw new CloudRuntimeException("cidr is not formatted correctly: "+ cidr);
}
final String cidrAddress = cidrPair[0];
final String cidrSize = cidrPair[1];
if (!isValidIp(cidrAddress)) {
- return null;
- }
- int cidrSizeNum = -1;
-
- try {
- cidrSizeNum = Integer.parseInt(cidrSize);
- } catch (final Exception e) {
- return null;
+ throw new CloudRuntimeException("cidr is not bvalid in ip space" + cidr);
}
- final long numericNetmask = 0xffffffff >> MAX_CIDR - cidrSizeNum << MAX_CIDR - cidrSizeNum;
+ long cidrSizeNum = getCidrSizeFromString(cidrSize);
+ final long numericNetmask = netMaskFromCidr(cidrSizeNum);
final long ipAddr = ip2Long(cidrAddress);
final Long[] cidrlong = {ipAddr & numericNetmask, (long)cidrSizeNum};
return cidrlong;
}
+ /**
+ * @param cidrSize
+ * @return
+ * @throws CloudRuntimeException
+ */
+ static long getCidrSizeFromString(final String cidrSize) throws CloudRuntimeException {
+ long cidrSizeNum = -1;
+
+ try {
+ cidrSizeNum = Integer.parseInt(cidrSize);
+ } catch (final NumberFormatException e) {
+ throw new CloudRuntimeException("cidrsize is not a valid int: " + cidrSize, e);
+ }
+ if(cidrSizeNum > 32 || cidrSizeNum < 0) {// assuming IPv4
+ throw new CloudRuntimeException("cidr size out of range: " + cidrSizeNum);
+ }
+ return cidrSizeNum;
+ }
+
--- End diff --
Same goes for this one. package (no modifier) vs private.
---
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: Cwe 190
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-158624231
@DaanHoogland It could be a result of another failure. On master this test passes, so maybe you can run just this test yourself and look at the details?
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1057#discussion_r44520633
--- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
@@ -869,31 +878,44 @@ public static boolean isNetworkAWithinNetworkB(final String cidrA, final String
public static Long[] cidrToLong(final String cidr) {
if (cidr == null || cidr.isEmpty()) {
- return null;
+ throw new CloudRuntimeException("empty cidr can not be converted to longs");
}
final String[] cidrPair = cidr.split("\\/");
if (cidrPair.length != 2) {
- return null;
+ throw new CloudRuntimeException("cidr is not formatted correctly: "+ cidr);
}
final String cidrAddress = cidrPair[0];
final String cidrSize = cidrPair[1];
if (!isValidIp(cidrAddress)) {
- return null;
- }
- int cidrSizeNum = -1;
-
- try {
- cidrSizeNum = Integer.parseInt(cidrSize);
- } catch (final Exception e) {
- return null;
+ throw new CloudRuntimeException("cidr is not bvalid in ip space" + cidr);
}
- final long numericNetmask = 0xffffffff >> MAX_CIDR - cidrSizeNum << MAX_CIDR - cidrSizeNum;
+ long cidrSizeNum = getCidrSizeFromString(cidrSize);
+ final long numericNetmask = netMaskFromCidr(cidrSizeNum);
final long ipAddr = ip2Long(cidrAddress);
final Long[] cidrlong = {ipAddr & numericNetmask, (long)cidrSizeNum};
return cidrlong;
}
+ /**
+ * @param cidrSize
+ * @return
+ * @throws CloudRuntimeException
+ */
+ static long getCidrSizeFromString(final String cidrSize) throws CloudRuntimeException {
+ long cidrSizeNum = -1;
+
+ try {
+ cidrSizeNum = Integer.parseInt(cidrSize);
+ } catch (final NumberFormatException e) {
+ throw new CloudRuntimeException("cidrsize is not a valid int: " + cidrSize, e);
+ }
+ if(cidrSizeNum > 32 || cidrSizeNum < 0) {// assuming IPv4
+ throw new CloudRuntimeException("cidr size out of range: " + cidrSizeNum);
+ }
+ return cidrSizeNum;
+ }
+
--- End diff --
I left it like this because the methods may have more use. They don't at the moment so closing them of could be done. For now this is fine by me. i haven't looked at implicit tests of these methods.
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-158327637
@remibergsma running them myself as well, now
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
GitHub user DaanHoogland reopened a pull request:
https://github.com/apache/cloudstack/pull/1057
Cwe 190
coverity warnings of this type adressed. Some where dismissed and maybe with reason but it seemed possible to remove them and hence obligatory ;p
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DaanHoogland/cloudstack CWE-190
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1057.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 #1057
----
commit b5625c05d04c7c886192ba9c3e8ed7c0b05ea482
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-11-10T13:00:22Z
CID-1175714 casts before bit shift
commit 637afb9b6740134b70d40ba8df4e93e36ded5b42
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-11-10T13:16:49Z
CID-1116485: cast cidr during bit shifting
and simple test included
commit 2fadfe93dcb63693485fb44a7c5146024e232c3c
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-11-10T13:40:24Z
CID-1116484 cast to long and use long as cidrsize type
and simpel test
commit f9d5c6918cf5f0b7c5524b4c2d421c84f3eb55ab
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-11-10T13:54:52Z
CID-1116483 cidr to netmask bitshifts guarded with casts
and simple test
commit 3ae4dd06f355ce81da4c672d1305c7707ccc6d8a
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-11-10T15:18:52Z
CID-1116482 cidrToLong cleanup of bitshift problem
and some trivial testing
commit 1bc837837ff39efc06f687cbb281ce97c08e4aee
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-11-11T09:05:47Z
CWE-190 netmask as long form cidr-size as method
commit 02058b939c54be8c088e57dbcc719bcfd5113ff0
Author: Daan Hoogland <da...@onecht.net>
Date: 2015-11-11T09:22:04Z
CWE-190 unit test for extremes of long netMaskFromCidr(long)
----
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1057#discussion_r44519906
--- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
@@ -869,31 +878,44 @@ public static boolean isNetworkAWithinNetworkB(final String cidrA, final String
public static Long[] cidrToLong(final String cidr) {
if (cidr == null || cidr.isEmpty()) {
- return null;
+ throw new CloudRuntimeException("empty cidr can not be converted to longs");
}
final String[] cidrPair = cidr.split("\\/");
if (cidrPair.length != 2) {
- return null;
+ throw new CloudRuntimeException("cidr is not formatted correctly: "+ cidr);
}
final String cidrAddress = cidrPair[0];
final String cidrSize = cidrPair[1];
if (!isValidIp(cidrAddress)) {
- return null;
- }
- int cidrSizeNum = -1;
-
- try {
- cidrSizeNum = Integer.parseInt(cidrSize);
- } catch (final Exception e) {
- return null;
+ throw new CloudRuntimeException("cidr is not bvalid in ip space" + cidr);
}
- final long numericNetmask = 0xffffffff >> MAX_CIDR - cidrSizeNum << MAX_CIDR - cidrSizeNum;
+ long cidrSizeNum = getCidrSizeFromString(cidrSize);
+ final long numericNetmask = netMaskFromCidr(cidrSizeNum);
final long ipAddr = ip2Long(cidrAddress);
final Long[] cidrlong = {ipAddr & numericNetmask, (long)cidrSizeNum};
return cidrlong;
}
+ /**
+ * @param cidrSize
+ * @return
+ * @throws CloudRuntimeException
+ */
+ static long getCidrSizeFromString(final String cidrSize) throws CloudRuntimeException {
+ long cidrSizeNum = -1;
+
+ try {
+ cidrSizeNum = Integer.parseInt(cidrSize);
+ } catch (final NumberFormatException e) {
+ throw new CloudRuntimeException("cidrsize is not a valid int: " + cidrSize, e);
+ }
+ if(cidrSizeNum > 32 || cidrSizeNum < 0) {// assuming IPv4
+ throw new CloudRuntimeException("cidr size out of range: " + cidrSizeNum);
+ }
+ return cidrSizeNum;
+ }
+
--- End diff --
I test them directly. Are you saying i shouldn't?
---
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: Cwe 190
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1057#issuecomment-158624430
I was busy starting ;)
---
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: Cwe 190
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1057#discussion_r44515580
--- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
@@ -775,11 +776,19 @@ public static String getSubNet(final String ip, final String netmask) {
}
public static String getCidrSubNet(final String ip, final long cidrSize) {
- final long numericNetmask = 0xffffffff >> MAX_CIDR - cidrSize << MAX_CIDR - cidrSize;
+ final long numericNetmask = netMaskFromCidr(cidrSize);
final String netmask = NetUtils.long2Ip(numericNetmask);
return getSubNet(ip, netmask);
}
+ /**
+ * @param cidrSize
+ * @return
+ */
+ static long netMaskFromCidr(final long cidrSize) {
+ return ((long)0xffffffff) >> MAX_CIDR - cidrSize << MAX_CIDR - cidrSize;
+ }
+
--- End diff --
Why are tho static methods using "package" modifier? Are classes in the same package going to access it? If the answer is no, they can be made "private".
---
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.
---