Fix NPE during public IP listing when a removed network or VPC ID is informed for associatenetworkid parameter by erikbocks · Pull Request #12372 · apache/cloudstack

erikbocks · 2026-01-05T17:10:15Z

Description

When listing public IPs associated to a network, the database query only considers records that are not marked as removed. If the ID of an removed resource is informed, a null value is returned in the database query. If the caller is a User type account, during the access checks the code tries to access one of the value's properties, resulting in an NullPointerException.

A validation was added to this flow to prevent the property access when the resource is null, and the listing behavior was standardized. The same validation was added to handle removed VPCs, as the same behavior was presented for them.

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)
Build/CI
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Bug Severity

Screenshots (if appropriate):

Behavior before the change

Behavior after the change

How Has This Been Tested?

First, I created an isolated network, informing a public IP address to be assigned as the network's source NAT. After that, the network was removed. Then, I authenticated with an User type account in CloudMonkey and called the listPublicIpAddresses API, informing the removed network's UUID as the associatednetworkid parameter. As expected, the exception was thrown.

Exception stack trace in Management Server's logs

2026-01-05 15:07:39,520 ERROR [c.c.a.ApiServer] (qtp1404565079-21:[ctx-8bd6ddc5, ctx-459e3280]) (logid:4b10ae2c) unhandled exception executing api command: [Ljava.lang.String;@1b1ed74b java.lang.NullPointerException: Cannot invoke "org.apache.cloudstack.acl.ControlledEntity.getDomainId()" because "entity" is null
	at com.cloud.user.AccountManagerImpl.checkAccess(AccountManagerImpl.java:738)
	at com.cloud.user.AccountManagerImpl.checkAccess(AccountManagerImpl.java:706)
	[...]
	at com.cloud.server.ManagementServerImpl.searchForIPAddresses(ManagementServerImpl.java:2651)

After applying the code with my changes to my local environment, the same steps were executed, and I validated that the behavior was fixed. The same steps were reproduced for removed VPCs.

The code was also compiled with tests on Maven.

codecov · 2026-01-05T17:22:43Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 4.15%. Comparing base (9ae696d) to head (3363136).
⚠️ Report is 11 commits behind head on 4.20.

❗ There is a different number of reports uploaded between BASE (9ae696d) and HEAD (3363136). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (9ae696d) HEAD (3363136)

unittests 1 0

Additional details and impacted files

@@              Coverage Diff              @@
##               4.20   #12372       +/-   ##
=============================================
- Coverage     16.26%    4.15%   -12.12%     
=============================================
  Files          5660      404     -5256     
  Lines        499963    32965   -466998     
  Branches      60708     5893    -54815     
=============================================
- Hits          81330     1370    -79960     
+ Misses       409559    31419   -378140     
+ Partials       9074      176     -8898

Flag	Coverage Δ
uitests	`4.15% <ø> (-0.01%)`	⬇️
unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

sureshanaparti · 2026-01-05T17:23:34Z

@blueorangutan package

blueorangutan · 2026-01-05T17:24:05Z

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

Copilot

Pull request overview

This PR fixes a NullPointerException that occurs when listing public IP addresses with a removed network or VPC ID as a parameter. When a User account attempts to list IPs using a removed resource ID, the database query returns null, causing an NPE during access checks.

Key Changes

Added null checks before accessing network and VPC objects when validating access permissions
Search criteria parameters are now conditionally set only when the associated resource exists
Added VpcVO import to support the VPC null check logic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

server/src/main/java/com/cloud/server/ManagementServerImpl.java

blueorangutan · 2026-01-05T19:09:44Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16258

DaanHoogland · 2026-01-06T07:45:03Z

@erikbocks , would this also apply to version 20 and 22? (not just main?)

winterhazel · 2026-02-03T11:17:26Z

@erikbocks as this is a bug fix, could you target 4.20?

erikbocks · 2026-02-04T14:54:10Z

@DaanHoogland @winterhazel, sure. The PR was re-targeted to the 4.20 branch.

DaanHoogland

clgtm

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-05T08:11:00Z

server/src/main/java/com/cloud/server/ManagementServerImpl.java

        if (associatedNetworkId != null) {
-            _accountMgr.checkAccess(caller, null, false, networkDao.findById(associatedNetworkId));
-            sc.setParameters("associatedNetworkIdEq", associatedNetworkId);
+            NetworkVO associatedNetwork = networkDao.findById(associatedNetworkId);
+
+            if (associatedNetwork != null) {
+                _accountMgr.checkAccess(caller, null, false, associatedNetwork);
+                sc.setParameters("associatedNetworkIdEq", associatedNetworkId);
+            }
        }
+
        if (vpcId != null) {
-            _accountMgr.checkAccess(caller, null, false, _vpcDao.findById(vpcId));
-            sc.setParameters("vpcId", vpcId);
+            VpcVO vpc = _vpcDao.findById(vpcId);
+
+            if (vpc != null) {
+                _accountMgr.checkAccess(caller, null, false, vpc);
+                sc.setParameters("vpcId", vpcId);
+            }
        }


The fix addresses the NPE on lines 2584-2598, but there's a similar vulnerability later in the same method. Lines 2614-2621 also retrieve the network using findById and directly access its properties (getDataCenterId() on line 2616 and getAccountId() on line 2621) without null checks. This code path is executed when listing free IP addresses (when isAllocatedOrReserved is false and vlanType is not DirectAttached). If a removed network ID is provided, this will still result in a NullPointerException.

The same null check pattern should be applied here to prevent this additional NPE scenario.

server/src/main/java/com/cloud/server/ManagementServerImpl.java

Copilot · 2026-02-05T08:11:00Z

server/src/main/java/com/cloud/server/ManagementServerImpl.java

        if (associatedNetworkId != null) {
-            _accountMgr.checkAccess(caller, null, false, networkDao.findById(associatedNetworkId));
-            sc.setParameters("associatedNetworkIdEq", associatedNetworkId);
+            NetworkVO associatedNetwork = networkDao.findById(associatedNetworkId);
+
+            if (associatedNetwork != null) {
+                _accountMgr.checkAccess(caller, null, false, associatedNetwork);
+                sc.setParameters("associatedNetworkIdEq", associatedNetworkId);
+            }
        }
+
        if (vpcId != null) {
-            _accountMgr.checkAccess(caller, null, false, _vpcDao.findById(vpcId));
-            sc.setParameters("vpcId", vpcId);
+            VpcVO vpc = _vpcDao.findById(vpcId);
+
+            if (vpc != null) {
+                _accountMgr.checkAccess(caller, null, false, vpc);
+                sc.setParameters("vpcId", vpcId);
+            }
        }


The fix changes the behavior of the listPublicIpAddresses API when given a removed network or VPC ID. Consider adding test cases to verify this new behavior, specifically:

Test that no NullPointerException is thrown when a User account calls listPublicIpAddresses with a removed network ID

Test that no NullPointerException is thrown when a User account calls listPublicIpAddresses with a removed VPC ID

Test the expected return value when a removed entity ID is provided (currently returns empty list)

The existing tests in ManagementServerImplTest.java test the setParameters method but don't cover the access control check scenario that was causing the NPE.

DaanHoogland · 2026-02-05T09:26:35Z

@blueorangutan package

blueorangutan · 2026-02-05T09:28:04Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-02-05T10:26:11Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16709

DaanHoogland · 2026-02-05T10:46:48Z

@blueorangutan test

blueorangutan · 2026-02-05T10:48:04Z

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2026-02-06T02:55:52Z

[SF] Trillian test result (tid-15384)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 55372 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr12372-t15384-kvm-ol8.zip
Smoke tests completed. 141 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

sureshanaparti · 2026-02-15T13:50:37Z

@blueorangutan package

blueorangutan · 2026-02-15T13:52:10Z

@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

boring-cyborg bot added the component:management-server label Jan 5, 2026

erikbocks changed the title ~~Fix NPE when listing public IPs associated to removed networks or VPCs~~ Fix NPE during public IP listing when a removed network or VPC ID is informed for associatenetworkid parameter Jan 5, 2026

sureshanaparti requested a review from Copilot January 5, 2026 17:23

Copilot started reviewing on behalf of sureshanaparti January 5, 2026 17:48 View session

Copilot AI reviewed Jan 5, 2026

View reviewed changes

server/src/main/java/com/cloud/server/ManagementServerImpl.java Show resolved Hide resolved

server/src/main/java/com/cloud/server/ManagementServerImpl.java Show resolved Hide resolved

server/src/main/java/com/cloud/server/ManagementServerImpl.java Show resolved Hide resolved

Fix NPE when listing public IPs associated to removed networks or VPCs

0d2f34a

erik-bock-silva force-pushed the fix-npe-when-listing-ips-of-removed-network branch from 3e34da6 to 0d2f34a Compare February 4, 2026 14:52

boring-cyborg bot added component:api component:storage component:UI labels Feb 4, 2026

erikbocks changed the base branch from main to 4.20 February 4, 2026 14:53

erikbocks removed component:api component:UI component:storage labels Feb 4, 2026

DaanHoogland requested a review from Copilot February 5, 2026 08:00

Copilot started reviewing on behalf of DaanHoogland February 5, 2026 08:00 View session

DaanHoogland approved these changes Feb 5, 2026

View reviewed changes

Copilot AI reviewed Feb 5, 2026

View reviewed changes

RosiKyu self-assigned this Feb 5, 2026

fix NPE in other associatenetworkid parameter use

3363136

Conversation

erikbocks commented Jan 5, 2026

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

codecov bot commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sureshanaparti commented Jan 5, 2026

Uh oh!

blueorangutan commented Jan 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

blueorangutan commented Jan 5, 2026

Uh oh!

DaanHoogland commented Jan 6, 2026

Uh oh!

winterhazel commented Feb 3, 2026

Uh oh!

erikbocks commented Feb 4, 2026

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Feb 5, 2026

Uh oh!

blueorangutan commented Feb 5, 2026

Uh oh!

blueorangutan commented Feb 5, 2026

Uh oh!

DaanHoogland commented Feb 5, 2026

Uh oh!

blueorangutan commented Feb 5, 2026

Uh oh!

blueorangutan commented Feb 6, 2026

Uh oh!

sureshanaparti commented Feb 15, 2026

Uh oh!

blueorangutan commented Feb 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Jan 5, 2026 •

edited

Loading