Prevent overflow on StatsCollector.java by GabrielBrascher · Pull Request #3932 · apache/cloudstack

GabrielBrascher · 2020-03-04T14:35:03Z

Description

Did a quick check and found an operation that can result in an overflow. Additionally, I took the liberty to do a couple of code enhancements/clean ups.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

GabrielBrascher · 2020-03-04T14:36:08Z

            s_logger.warn("Usage stats job aggregation range is to small, using the minimum value of " + UsageUtils.USAGE_AGGREGATION_RANGE_MIN);
            _usageAggregationRange = UsageUtils.USAGE_AGGREGATION_RANGE_MIN;
        }
-        _diskStatsUpdateExecutor.scheduleAtFixedRate(new VmDiskStatsUpdaterTask(), (endDate - System.currentTimeMillis()), (_usageAggregationRange * 60 * 1000),


Potential overflow in 2 Values before it is converted to long by use in an invocation context.

GabrielBrascher · 2020-03-04T14:37:08Z

            for (AutoScaleVmGroupPolicyMapVO asVmgPmap : listMap) {
                AutoScalePolicyVO policyVO = _asPolicyDao.findById(asVmgPmap.getPolicyId());
                if (policyVO != null) {
-                    Integer quitetime = policyVO.getQuietTime();


The variable 'quitetime' is only assigned values of primitive type and is never 'null', but it is declared with the boxed type 'Integer'.

GabrielBrascher · 2020-03-04T15:57:34Z

@blueorangutan package

blueorangutan · 2020-03-04T15:58:06Z

@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-03-04T16:24:28Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1000

yadvr · 2020-03-05T06:42:53Z

@blueorangutan test

blueorangutan · 2020-03-05T06:43:04Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-03-05T15:06:04Z

Trillian test result (tid-1191)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28797 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3932-t1191-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
Smoke tests completed. 81 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

weizhouapache

code LGTM

GabrielBrascher · 2020-03-06T13:29:34Z

Thanks for all the reviews!

Just to mention. I did a few manual tests to ensure that no overflow will happen. Setting ONE_MINUTE_IN_MILLISCONDS to Long (or long) will indeed solve overflow issues.

DaanHoogland · 2020-03-07T08:50:13Z

@GabrielBrascher as this is a pure fix, will it apply to 4.13?

borisstoyanov

@GabrielBrascher can you share some steps to reproduce this? I'm trying to find out what exactly we're trying to fix here.

GabrielBrascher · 2020-03-09T12:00:06Z

@borisstoyanov the overflow is not something that will happen easily; however, the code is there and it still something that might happen one day depending on the value stored at _usageAggregationRange.

The operation that I extracted can result in an overflow as it is an integer operation stored in an integer memory (and then after storing it as an integer it is cast into a long). I extracted it into a long = int * Long, which prevents a overflow as it is not an integer operation anymore but converted into a long operation.

GabrielBrascher · 2020-03-09T12:01:37Z

@DaanHoogland I see no problem in applying this at 4.13.1.

DaanHoogland · 2020-03-09T12:10:29Z

ok, can you rebase @GabrielBrascher ?

GabrielBrascher · 2020-03-09T12:46:53Z

@DaanHoogland rebased it to 4.13 🙂

borisstoyanov · 2020-03-10T09:18:55Z

@blueorangutan package

blueorangutan · 2020-03-10T09:19:31Z

@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-03-10T09:37:32Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1037

borisstoyanov · 2020-03-10T09:38:23Z

@blueorangutan test

blueorangutan · 2020-03-10T09:38:30Z

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-03-10T19:26:45Z

Trillian test result (tid-1225)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33837 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3932-t1225-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Smoke tests completed. 76 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	`Failure`	219.74	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	210.20	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	942.54	test_privategw_acl.py

andrijapanicsb · 2020-03-13T18:51:07Z

2 x LGTMs, all tests passing (those privategtw tests are "OK", fixed in newer ACS code - this PR is 150+ commits behind master)

Merging.

GabrielBrascher added type:bug type:enhancement type:cleanup labels Mar 4, 2020

GabrielBrascher self-assigned this Mar 4, 2020

GabrielBrascher force-pushed the statscollector-potential-issues branch from 334864f to 33007f2 Compare March 4, 2020 14:56

GabrielBrascher commented Mar 4, 2020

View reviewed changes

GabrielBrascher removed the type:cleanup label Mar 4, 2020

yadvr added this to the 4.14.0.0 milestone Mar 5, 2020

DaanHoogland closed this Mar 6, 2020

DaanHoogland reopened this Mar 6, 2020

GabrielBrascher changed the title ~~Prevent overflow and null pointer on StatsCollector.java~~ Prevent overflow on StatsCollector.java Mar 6, 2020

weizhouapache approved these changes Mar 6, 2020

View reviewed changes

DaanHoogland approved these changes Mar 6, 2020

View reviewed changes

borisstoyanov reviewed Mar 9, 2020

View reviewed changes

GabrielBrascher changed the base branch from master to 4.13 March 9, 2020 11:47

GabrielBrascher changed the base branch from 4.13 to master March 9, 2020 11:48

GabrielBrascher changed the base branch from master to 4.13 March 9, 2020 12:32

GabrielBrascher force-pushed the statscollector-potential-issues branch from ff95beb to 38755ed Compare March 9, 2020 12:44

GabrielBrascher force-pushed the statscollector-potential-issues branch from 38755ed to c4ea2ad Compare March 9, 2020 12:52

Prevent overflow on StatsCollector + add a few enhancements on code

b9aad0f

GabrielBrascher force-pushed the statscollector-potential-issues branch from c4ea2ad to b9aad0f Compare March 9, 2020 13:03

DaanHoogland modified the milestones: 4.14.0.0, 4.13.1.0 Mar 10, 2020

andrijapanicsb merged commit cd6f0cb into apache:4.13 Mar 13, 2020

Conversation

GabrielBrascher commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Uh oh!

GabrielBrascher Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GabrielBrascher commented Mar 4, 2020

Uh oh!

blueorangutan commented Mar 4, 2020

Uh oh!

blueorangutan commented Mar 4, 2020

Uh oh!

yadvr commented Mar 5, 2020

Uh oh!

blueorangutan commented Mar 5, 2020

Uh oh!

blueorangutan commented Mar 5, 2020

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Mar 6, 2020

Uh oh!

DaanHoogland commented Mar 7, 2020

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Mar 9, 2020

Uh oh!

GabrielBrascher commented Mar 9, 2020

Uh oh!

DaanHoogland commented Mar 9, 2020

Uh oh!

GabrielBrascher commented Mar 9, 2020

Uh oh!

borisstoyanov commented Mar 10, 2020

Uh oh!

blueorangutan commented Mar 10, 2020

Uh oh!

blueorangutan commented Mar 10, 2020

Uh oh!

borisstoyanov commented Mar 10, 2020

Uh oh!

blueorangutan commented Mar 10, 2020

Uh oh!

blueorangutan commented Mar 10, 2020

Uh oh!

andrijapanicsb commented Mar 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

GabrielBrascher commented Mar 4, 2020 •

edited

Loading