Reject duplicate rlimit types in process spec

[pujitha24]

The OCI runtime-spec (config.md) says: "If rlimits contains duplicated
entries with same type, the runtime MUST generate an error." runc did not
check this: a config with two RLIMIT_NOFILE entries was accepted and the
last entry silently took effect.

Approach:

• configs.CheckRlimits returns an error for duplicate types.
• It is called from validate.Validate (for config.Rlimits) and from
  Container.start for init processes only, so create/run reject a spec
  with duplicates.
• runc exec into an existing container is unaffected (a config.json
  created by an older runc may have duplicates). runc exec --process
  is new input, so it is checked in getProcess via checkProcessRlimits.
• runc restore does not go through start(). It does go through
  validate.Validate, but the runc CLI never sets config.Rlimits (specconv
  doesn't), so that check is a no-op there; only libcontainer API users that
  set Config.Rlimits are affected.

Behavior change: a spec with duplicate rlimit types now fails with
duplicate rlimit type: <numeric type> instead of starting.

Testing:

• Unit tests TestValidateRlimits and TestCheckProcessRlimits pass on
  Linux arm64 (Lima VM, Go 1.26).
• Added bats tests in exec.bats (exec into an existing container whose
  config.json has duplicates still works; exec --process and run with
  duplicates fail). I could not run bats here, so these have not been run
  yet.

Fixes #5493

[kolyshkin]

This is not the way to go:

• runc exec will re-validate the existing config.json and fail to run (on an existing container which worked before). This is not acceptable.
• Same for runc restore.

The validation belongs to libcontainer.

Also, the CHANGELOG entry should say "Changed" not "Fixed" because this is a change in behavior.

nit: add this PR number to changelog entry.

[kolyshkin]

@pujitha24 please do not add more commits on top of the existing one, this does not make sense. We want to see how the code/commits look like when it's merged. Feel free to force-push to your branch.

[pujitha24]

That's fixed in 20b5fd8. The duplicate check is no longer in validateProcessSpec. It now runs in validate.Validate for config.Rlimits at container creation, and in Container.start for a new process's Rlimits. So runc exec and runc restore don't re-validate an existing config.json. I moved the changelog entry under "Changed" and added #5494 next to #5493. I haven't run the tests on this machine (it's macOS and this code is Linux-only), so I'm relying on CI for the new validator_test.go cases.

[kolyshkin]

Thanks for moving the check into libcontainer. A few things still need fixing:

1. runc exec into an existing container can still break. Without --process, runc exec reads spec.Process from the bundle's config.json (see getProcess in exec.go), converts its rlimits, and passes them to c.start(), which now calls CheckRlimits. So if an older runc created a container from a config with duplicate rlimits, you can no longer exec into it. That is the regression I mentioned earlier; it has just moved to a different place. Also, the CHANGELOG says "Existing containers are not re-validated", which is not true for exec.

   One way to fix this: in start(), only check when process.Init is true, and for exec, check only on the --process path (validateProcessSpec), since that is where the input is new. restore does not go through start(), and the runc CLI never sets config.Rlimits, so neither of those is affected.

2. Please squash the commits. The second commit ("Move duplicate rlimit check into libcontainer") is still on top.

3. The PR description is out of date. It still mentions validateProcessSpec and the error message duplicate rlimit type: RLIMIT_NOFILE, but the code now prints the numeric type.

4. Tests. Only Validate(config) is tested. Please add a test showing that exec into an existing container still works (point 1).

5. nit: CheckRlimits can be simpler. There are at most 16 rlimit types (RLIM_NLIMITS), so a linear search over the entries already checked is shorter, faster, and never allocates:

   func CheckRlimits(limits []Rlimit) error {
   	for i, l := range limits {
   		if slices.ContainsFunc(limits[:i], func(r Rlimit) bool { return r.Type == l.Type }) {
   			return fmt.Errorf("duplicate rlimit type: %d", l.Type)
   		}
   	}
   	return nil
   }

   Benchmark (i7-12800H, Go 1.26, all types unique, so each check scans the whole slice):

   n	map	search	sort
   0	8.2 ns	2.0 ns	5.7 ns
   1	13.9 ns	3.1 ns	5.9 ns
   2	22.2 ns	5.5 ns	8.8 ns
   4	38.7 ns	8.9 ns	15.0 ns
   8	84.1 ns	21.6 ns	54.0 ns, 1 alloc / 64 B
   16	326.5 ns, 3 allocs / 616 B	57.6 ns	91.6 ns, 1 alloc / 128 B

   In practice these differences don't matter, but the search version is also the shortest.

Benchmark code

package rlbench

import (
	"fmt"
	"slices"
	"testing"
)

type Rlimit struct {
	Type       int
	Hard, Soft uint64
}

func checkMap(limits []Rlimit) error {
	seen := make(map[int]struct{}, len(limits))
	for _, l := range limits {
		if _, ok := seen[l.Type]; ok {
			return fmt.Errorf("duplicate rlimit type: %d", l.Type)
		}
		seen[l.Type] = struct{}{}
	}
	return nil
}

func checkSearch(limits []Rlimit) error {
	for i, l := range limits {
		if slices.ContainsFunc(limits[:i], func(r Rlimit) bool { return r.Type == l.Type }) {
			return fmt.Errorf("duplicate rlimit type: %d", l.Type)
		}
	}
	return nil
}

func checkSort(limits []Rlimit) error {
	types := make([]int, len(limits))
	for i, l := range limits {
		types[i] = l.Type
	}
	slices.Sort(types)
	for i := 1; i < len(types); i++ {
		if types[i] == types[i-1] {
			return fmt.Errorf("duplicate rlimit type: %d", types[i])
		}
	}
	return nil
}

var sink error

func Benchmark(b *testing.B) {
	for _, n := range []int{0, 1, 2, 4, 8, 16} {
		l := make([]Rlimit, n)
		for i := range l {
			l[i].Type = n - 1 - i // unique, reverse order
		}
		for _, f := range []struct {
			name string
			fn   func([]Rlimit) error
		}{{"map", checkMap}, {"search", checkSearch}, {"sort", checkSort}} {
			b.Run(fmt.Sprintf("%s/n=%d", f.name, n), func(b *testing.B) {
				b.ReportAllocs()
				for b.Loop() {
					sink = f.fn(l)
				}
			})
		}
	}
}

[pujitha24]

You're right, that would have broken exec and restore on existing containers. I dropped the check from the spec-conversion path and moved it into libcontainer: configs.Validate now rejects duplicate rlimit types when a container is created, and Container.start checks Process.Rlimits for new processes, so existing config.json files aren't re-validated on restore. The CHANGELOG entry is now under "Changed" and has the PR numbers. I only checked that this compiles for linux and vets cleanly (the new TestValidateRlimits needs Linux, and I'm on macOS), so I haven't run it — CI will be the first real run.

[pujitha24]

@kolyshkin I've pushed changes addressing your review — the branch is now at 20b5fd8 and CI is green. Could you take another look when you have a moment? Happy to keep iterating if anything is still off.

[kolyshkin]

@pujitha24 I've asked before, and I'll ask again: please squash your commits.

[kolyshkin]

Looks like my other review comments (#5494 (comment)) are still not addressed. PTAL @pujitha24

[kolyshkin]

I don't like that we convert just to validate. Lot of garbage to collect.

Can be checked right here without extra allocations:

// checkProcessRlimits returns an error if p has duplicate rlimit types.
func checkProcessRlimits(p *specs.Process) error {
      for i, r := range p.Rlimits {
              if slices.ContainsFunc(p.Rlimits[:i], func(x specs.POSIXRlimit) bool { return x.Type == r.Type }) {
                      return fmt.Errorf("duplicate rlimit type: %s", r.Type)
              }
      }
      return nil
}

[kolyshkin]

I think this check should be done after container already has init process.

[kolyshkin]

@pujitha24 please squash your commits

[pujitha24]

Both inline points are done in d95e649: checkProcessRlimits now compares specs.POSIXRlimit types directly with slices.ContainsFunc, with no conversion to configs.Rlimit, and the Container.start check moved to after the container already has init process check. I dropped the "unknown type" test case since that path no longer goes through createLibContainerRlimit.

On runc exec/restore re-validating an existing config.json: the config-level check lives in validate.Validate and runs only at create, and the Container.start check is init-only, so exec into an existing container isn't re-checked. The only thing runc exec checks is the --process file, since that's new input. If you'd rather that live entirely in libcontainer, I can drop the check in exec.go. The changelog entry is under "Changed" and has this PR's number.

One caveat: I'm on macOS, so I could only run go vet on libcontainer/configs/.... I couldn't build or run the root-package tests (cgo/linux-only), so please rely on CI for those.