update.go: fix nil pointer risk in Intel RDT initialization #4774

AntonMoryakov · 2025-05-28T19:50:51Z

Static analysis revealed potential nil dereference when applying RDT
configurations. The intelRdtManager instance from NewManager() could be
nil but was used without check in Apply() call.

Added explicit nil check that returns error to fail safely rather than
panic. This matches error handling patterns used elsewhere in the codebase.

Signed-off-by: Anton Moryakov [email protected]

AntonMoryakov · 2025-05-28T19:59:47Z

Hi!

While working on this PR, I noticed that the CI fails due to SA5011 reports from staticcheck, but these originate from unrelated test files in the current codebase (not modified in this PR).

Example:

if l.Level != expectedLogs.Level {

This triggers:

SA5011: possible nil pointer dereference

This can be safely resolved using:

if l != nil && l.Level != expectedLogs.Level {

There are similar occurrences in:

libcontainer/capabilities/capabilities_linux_test.go
libcontainer/configs/tocpuset_test.go
libcontainer/system/kernelversion/kernel_linux_test.go

Let me know if you'd like me to submit a cleanup PR to address these 🙌

Thanks!
– Anton

lifubang · 2025-05-29T10:20:38Z

update.go

@@ -363,6 +363,9 @@ other options are ignored.
 				}
 				config.IntelRdt = &configs.IntelRdt{}
 				intelRdtManager := intelrdt.NewManager(&config, container.ID(), state.IntelRdtPath)
+				if intelRdtManager == nil {


Thanks your contribution.
I think the only position of returning nil is here:

runc/libcontainer/intelrdt/intelrdt.go

Lines 162 to 165 in 314b79a

if _, err := Root(); err != nil {

// Intel RDT is not available.

return nil

}

But unfortunately, we didn't return the error to the caller, so I think we should add an error log in L163 to let the user know what's happened. WDYT?

Good catch — you're right, NewManager() can return nil from intelrdt.go when Root() fails.
I've added a log message at that point in update.go to inform the user when RDT is unavailable.
Pushed the change, thanks again!

lifubang · 2025-05-29T11:08:34Z

update.go

@@ -363,6 +363,10 @@ other options are ignored.
 				}
 				config.IntelRdt = &configs.IntelRdt{}
 				intelRdtManager := intelrdt.NewManager(&config, container.ID(), state.IntelRdtPath)
+				if intelRdtManager == nil {
+					logrus.Errorf("Intel RDT is not available: resctrl filesystem not found or unsupported")


I think you misunderstood my comment, you should add the log in here:

runc/libcontainer/intelrdt/intelrdt.go

Lines 162 to 165 in 314b79a

if _, err := Root(); err != nil {

// Intel RDT is not available.

return nil

}

Because no one knows what’s contents in this ‘err’.

For example:

logrus.Errorf("Intel RDT is not available: %v", err)

Static analysis revealed a possible nil dereference when applying Intel RDT configurations. The `NewManager()` function can return nil if Intel RDT is not supported by the kernel (e.g. no resctrl filesystem mounted), but the caller in `update.go` did not check for this condition. This patch adds an explicit nil check after NewManager() is called, with a descriptive log message to help users understand the failure. In addition, `NewManager()` itself now logs the underlying reason (using the error from `Root()`), improving observability. This issue was originally identified via static analysis: > update.go:367:9: undefined: log > and: Return value of function 'NewManager' will be overwritten Signed-off-by: AntonMoryakov <[email protected]>

lifubang · 2025-06-02T11:32:42Z

update.go

@@ -363,6 +363,10 @@ other options are ignored.
 				}
 				config.IntelRdt = &configs.IntelRdt{}
 				intelRdtManager := intelrdt.NewManager(&config, container.ID(), state.IntelRdtPath)
+				if intelRdtManager == nil {
+					logrus.Errorf("Intel RDT manager creation failed: likely due to missing resctrl filesystem")


This line should be removed.

lifubang · 2025-06-02T11:34:41Z

libcontainer/intelrdt/intelrdt.go

@@ -161,6 +162,7 @@ func NewManager(config *configs.Config, id string, path string) *Manager {
 	}
 	if _, err := Root(); err != nil {
 		// Intel RDT is not available.
+		logrus.Errorf("Intel RDT is not available: %v", err)


I'm sorry to say that, I think we should remove this line now. Please see my comment as follows.

If we want to keep this, we should return error in update.go, for example:

if intelRdtManager == nil { return fmt.Erroff("Intel RDT manager creation failed: likely due to missing resctrl filesystem") }

lifubang · 2025-06-02T11:37:46Z

update.go

@@ -363,6 +363,10 @@ other options are ignored.
 				}
 				config.IntelRdt = &configs.IntelRdt{}
 				intelRdtManager := intelrdt.NewManager(&config, container.ID(), state.IntelRdtPath)
+				if intelRdtManager == nil {
+					logrus.Errorf("Intel RDT manager creation failed: likely due to missing resctrl filesystem")
+					return nil


We should not return nil here. I think the entire logic should be changed like this:

if intelRdtManager != nil && err := intelRdtManager.Apply(state.InitProcessPid); err != nil {

Like my comment in #4774 (comment)
I think that one is more reasonable, WDYT?

kolyshkin

NACK

A nil dereference is not possible here, because in update.go we call intelrdt.IsCATEnabled and intelrdt.IsMBAEnabled earlier, and if Root returns nil, we'll error out here:

if l3CacheSchema != "" && !intelrdt.IsCATEnabled() {
    return errors.New("Intel RDT/CAT: l3 cache schema is not enabled")
}
  
if memBwSchema != "" && !intelrdt.IsMBAEnabled() {
     return errors.New("Intel RDT/MBA: memory bandwidth schema is not enabled")
}

AntonMoryakov · 2025-06-05T02:05:48Z

NACK

A nil dereference is not possible here, because in update.go we call intelrdt.IsCATEnabled and intelrdt.IsMBAEnabled earlier, and if Root returns nil, we'll error out here:
if l3CacheSchema != "" && !intelrdt.IsCATEnabled() {
    return errors.New("Intel RDT/CAT: l3 cache schema is not enabled")
}
  
if memBwSchema != "" && !intelrdt.IsMBAEnabled() {
     return errors.New("Intel RDT/MBA: memory bandwidth schema is not enabled")
}

Thanks for the feedback and the detailed explanation!

You’re absolutely right — after reviewing the call chain and how intelrdt.IsCATEnabled() and intelrdt.IsMBAEnabled() rely on Root(), I agree that this is a false positive from the static analyzer.
The nil dereference is not possible in this context, as the failure is already handled earlier during feature checks.

Can we close this PR then? Thanks again for the clarification!

lifubang · 2025-06-05T10:59:21Z

I agree that this is a false positive from the static analyzer.

Which static analysis tool did you use? I use 'go vet ./...' to test, I can't see any static check errors.

lifubang · 2025-06-10T01:37:27Z

NACK

In practice, this nil check isn't strictly necessary.

However, as a good coding practice, I think it's worthwhile to include a nil check here. It helps prevent potential errors when refactoring this code in the future.

It's just my own opinion, please feel free to close it.

AntonMoryakov · 2025-06-10T10:00:04Z

Which static analysis tool did you use? I use 'go vet ./...' to test, I can't see any static check errors.

static analyzers like SVACE

AntonMoryakov force-pushed the DEREF_OF_NULL.RET-in-update.go branch from 53a77e5 to 314b79a Compare May 28, 2025 19:52

lifubang reviewed May 29, 2025

View reviewed changes

AntonMoryakov force-pushed the DEREF_OF_NULL.RET-in-update.go branch 2 times, most recently from d12a3f2 to 94aaf27 Compare May 29, 2025 10:45

AntonMoryakov requested a review from lifubang May 29, 2025 10:51

lifubang reviewed May 29, 2025

View reviewed changes

AntonMoryakov force-pushed the DEREF_OF_NULL.RET-in-update.go branch from 94aaf27 to 290b43b Compare June 1, 2025 19:55

AntonMoryakov force-pushed the DEREF_OF_NULL.RET-in-update.go branch from 290b43b to d3505b9 Compare June 1, 2025 19:58

lifubang requested changes Jun 2, 2025

View reviewed changes

kolyshkin reviewed Jun 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

update.go: fix nil pointer risk in Intel RDT initialization #4774

update.go: fix nil pointer risk in Intel RDT initialization #4774

Uh oh!

AntonMoryakov commented May 28, 2025

Uh oh!

AntonMoryakov commented May 28, 2025

Uh oh!

lifubang May 29, 2025

Uh oh!

AntonMoryakov May 29, 2025

Uh oh!

lifubang May 29, 2025

Uh oh!

lifubang May 30, 2025

Uh oh!

lifubang Jun 2, 2025

Uh oh!

lifubang Jun 2, 2025

Uh oh!

lifubang Jun 2, 2025

Uh oh!

lifubang Jun 2, 2025

Uh oh!

lifubang Jun 2, 2025

Uh oh!

kolyshkin left a comment

Uh oh!

AntonMoryakov commented Jun 5, 2025

Uh oh!

lifubang commented Jun 5, 2025

Uh oh!

lifubang commented Jun 10, 2025

Uh oh!

AntonMoryakov commented Jun 10, 2025

Uh oh!

Uh oh!

	if _, err := Root(); err != nil {
	// Intel RDT is not available.
	return nil
	}

update.go: fix nil pointer risk in Intel RDT initialization #4774

Are you sure you want to change the base?

update.go: fix nil pointer risk in Intel RDT initialization #4774

Uh oh!

Conversation

AntonMoryakov commented May 28, 2025

Uh oh!

AntonMoryakov commented May 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

AntonMoryakov commented Jun 5, 2025

Uh oh!

lifubang commented Jun 5, 2025

Uh oh!

lifubang commented Jun 10, 2025

Uh oh!

AntonMoryakov commented Jun 10, 2025

Uh oh!

Uh oh!