Skip to content

fix: replace dead len > 2 guard with len != 2 in addSysctl#6883

Open
immanuwell wants to merge 1 commit into
podman-container-tools:mainfrom
immanuwell:fix/sysctl-splitn-dead-check
Open

fix: replace dead len > 2 guard with len != 2 in addSysctl#6883
immanuwell wants to merge 1 commit into
podman-container-tools:mainfrom
immanuwell:fix/sysctl-splitn-dead-check

Conversation

@immanuwell

@immanuwell immanuwell commented May 29, 2026

Copy link
Copy Markdown

What type of PR is this?

/kind bug

What this PR does / why we need it:

strings.SplitN(s, "=", 2) returns at most 2 elements, so len(splitn) > 2 in addSysctl is dead code that never fires. When a sysctl in containers.conf has no =, splitn has length 1 and splitn[1] panics instead of hitting the error return right below.

Changing > 2 to != 2 kills the dead code and catches the bad input.

How to verify it

# ~/.config/containers/containers.conf
[containers]
default_sysctls = ["net.core.rmem_max"]

Before: buildah run panics with index out of range [1] with length 1
After: returns sysctl "net.core.rmem_max" defined in containers.conf must be formatted name=value

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

strings.SplitN(s, "=", 2) returns at most 2 elements, so the
existing len > 2 check never fires. When a sysctl in containers.conf
has no '=' the slice has length 1, and the unconditional access to
splitn[1] panics. Change the guard to != 2 so a malformed sysctl
returns the descriptive error instead of an index-out-of-bounds panic.

Signed-off-by: Immanuel Tikhonov <pchpr.00@list.ru>
Signed-off-by: immanuwell <pchpr.00@list.ru>

@nalind nalind left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original version was probably intended to be <, but this works, too.
LGTM, thanks!

@TomSweeneyRedHat

Copy link
Copy Markdown
Contributor

LGTM

@nalind nalind enabled auto-merge May 29, 2026 18:59
@nalind nalind disabled auto-merge May 29, 2026 19:55
@TomSweeneyRedHat

Copy link
Copy Markdown
Contributor

/lgtm

@Honny1 Honny1 added the No New Tests Allow PR to proceed without adding regression tests label Jun 12, 2026

@Honny1 Honny1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, can you please rebase on upstream main to enable new CI?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

No New Tests Allow PR to proceed without adding regression tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants