Skip to content

fix(cambricon): clamp oversized memory request before int32 conversion #2339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shivv23 wants to merge 1 commit into from
+46 −1
Closed

fix(cambricon): clamp oversized memory request before int32 conversion #2339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion pkg/device/cambricon/device.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"encoding/json"
"flag"
"fmt"
"math"
"math/rand"
"slices"
"strings"
Expand Down Expand Up @@ -251,7 +252,12 @@ func (dev *CambriconDevices) GenerateResourceRequests(ctr *corev1.Container) dev
memnums, ok := mem.AsInt64()
klog.Infoln("mluResourceMem", mem, memnums)
if ok {
memnum = int(memnums) * 256
if memnums > math.MaxInt32/256 || memnums < 0 {
klog.ErrorS(nil, "cambricon memory request is out of int32 range, clamping to max int32", "container", ctr.Name, "request", memnums)
memnum = math.MaxInt32
} else {
memnum = int(memnums * 256)
}
Comment on lines +255 to +260

@coderabbitai coderabbitai Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate file =="
fd -a 'device.go' pkg/device/cambricon || true

echo "== git diff stat =="
git diff --stat || true

echo "== relevant imports/context =="
sed -n '1,120p' pkg/device/cambricon/device.go
echo "== target lines =="
sed -n '220,300p' pkg/device/cambricon/device.go

echo "== quantity usage in cambricon/device.go =="
rg -n "mem\.AsInt64\(|PercentageRequest|MemPercentagereq|Fit\(|Quantity|ResourceList|ResourceRequests" pkg/device/cambricon/device.go

echo "== module kubernetes/apimachinery version =="
if [ -f go.mod ]; then
  rg -n "apimachinery|k8s.io/apimachinery" go.mod
fi

Repository: Project-HAMi/HAMi

Length of output: 7983

🌐 Web query:

kubernetes apimachinery resource Quantity AsInt64 negative scale MilliValue validation

💡 Result:

In Kubernetes apimachinery, the resource.Quantity type uses a fixed-point representation to avoid floating-point errors [1]. When handling Quantity values, specifically regarding AsInt64, MilliValue, and negative scales, the following behaviors and validation considerations apply: 1. Rounding and Negative Values: Kubernetes Quantity operations such as MilliValue() and ScaledValue() are designed to round toward "positive infinity" (away from zero) for positive numbers and toward "negative infinity" (away from zero) for negative numbers [2]. Historically, bugs in the implementation (e.g., scaledValue) caused some small negative values to round incorrectly toward positive infinity [2]. Recent fixes have ensured these operations correctly round away from zero for negative inputs [2]. 2. AsInt64 Behavior: The AsInt64() method returns an int64 at scale 0 [3]. It will return false if the value cannot be represented as an int64 (e.g., it exceeds math.MaxInt64 or math.MinInt64) or if the conversion would result in a loss of precision (i.e., the value is fractional) [3]. 3. Validation: Because Value() and MilliValue() project through int64, they can overflow if the Quantity is very large [2][4]. Validating these values by checking against resource.MaxMilliValue is considered unreliable because MaxMilliValue itself is a constant that may not account for fractional quantities or the full range of Quantity [4]. Instead, the recommended approach is to use Quantity.Cmp() to compare against a defined bound, such as the resource.MaxMilliQuantity() (introduced to provide a safe, exact bound for MilliValue checks) [4]. 4. Negative Scales: The internal int64Amount structure handles scales, where a negative scale (e.g., scale = -3 for milli-quantities) effectively represents fractional values [5][6]. When performing operations with these scales, the implementation ensures that quantities remain representable as fixed-point numbers [1][3]. For developers, it is critical to perform sign-aware checks (e.g., q.Sign() >= 0) and use Cmp() for bounds checking to avoid overflows and incorrect rounding behavior when working with large or negative quantities [4].

Citations:

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Go package availability =="
go list -m -f '{{.Version}}' k8s.io/apimachinery 2>/dev/null || true
go env GOPATH 2>/dev/null || true

echo "== inspect vendored/source Quantity.AsInt64 if present =="
find "$(go env GOPATH 2>/dev/null || true)/pkg/mod/k8s.io/apimachinery" -path '*pkg/api/resource/amount.go' -type f 2>/dev/null | head -20 || true

python3 - <<'PY'
from pathlib import Path
import re, subprocess

base = subprocess.check_output(["go", "env", "GOPATH"], text=True).strip()
paths = subprocess.check_output(
    ["find", f"{base}/pkg/mod/k8s.io/apimachinery", "-path", "*pkg/api/resource/amount.go", "-type", "f"],
    text=True, stderr=subprocess.DEVNULL
).splitlines()[:1]

for p in paths:
    print(f"PATH={p}")
    txt = Path(p).read_text()
    idx = txt.find("func (i *int64Amount) AsInt64() (int64, bool)")
    if idx != -1:
        snippet = txt[idx:]
        print(snippet[:snippet.find("\n\nfunc ")+500] if "\n\nfunc " in snippet[idx+1:] else snippet[:800])
PY

python3 - <<'PY'
import subprocess, textwrap, re

try:
    out = subprocess.check_output(["go", "env", "GOPATH"], text=True).strip()
except Exception as e:
    print(f"go env failed: {e}")
    raise SystemExit(0)

paths = subprocess.check_output(
    ["find", f"{out}/pkg/mod/k8s.io/apimachinery-v0.36.3", "-path", "*pkg/api/resource/quantity.go", "-type", "f"],
    text=True, stderr=subprocess.DEVNULL
).splitlines()

for p in paths:
    txt = Path(p).read_text()
    for fn in ["func (q Quantity) AsInt64()", "func (q Quantity) MilliValue()", "func ValidateResourceFieldPath"]:
        idx = txt.find(fn)
        if idx != -1:
            end = txt.find("\nfunc ", idx+1)
            if end == -1: end = idx+3000
            print(f"=== {p} :: {fn} ===")
            print(txt[idx:end])
PY

echo "== validate Quantity negative scale behavior by compiling vendor source into temporary package? =="
# Avoid executing repository code. Only inspect generated code/import behavior if tooling exists.
tmpdir="$(mktemp -d)"
cat > "$tmpdir/main.go" <<'GO'
package main

import (
	"fmt"
	"k8s.io/apimachinery/pkg/api/resource"
)

func main() {
	for _, s := range []string{"10000000.0", "1.1", "-1"} {
		q, err := resource.ParseQuantity(s)
		if err != nil {
			fmt.Println(s, "ERROR", err)
			continue
		}
		v, ok := q.AsInt64()
		fmt.Println(s, "AsInt64(ok)=", ok, "value=", v, "format=", q.Format, "internal=", q.Value(), "Milli=", q.MilliValue())
	}
}
GOCACHE="$tmpdir/gocache" go env -w GOMODCACHE="$tmpdir/modules"
GOCACHE="$tmpdir/gocache" go -C "$tmpdir" run main.go || true
GOCACHE="$tmpdir/gocache" go env -w GOMODCACHE= 2>/dev/null || true

echo "== Fit behavior =="
sed -n '310,390p' pkg/device/cambricon/device.go

Repository: Project-HAMi/HAMi

Length of output: 782

🏁 Script executed:

#!/bin/bash
set -u

echo "== GOPATH and module cache =="
go env GOPATH GOMODCACHE 2>/dev/null || true

echo "== all apimachinery package files =="
find "$HOME/go/pkg/mod" -path '*k8s.io/apimachinery*' -type f 2>/dev/null | head -50 || true

python3 - <<'PY'
from pathlib import Path
import subprocess, re

gop = Path(subprocess.check_output(["go", "env", "GOPATH"], text=True, stderr=subprocess.DEVNULL).strip())
candidates = sorted(glob := (gop / "pkg/mod" / "k8s.io/apimachinery-v0.36.3").glob("*/pkg/api/resource/amount.go")) if gop.exists() else []
print("candidates:", candidates)
for p in candidates:
    txt = p.read_text()
    print(f"FILE {p} AsInt64:")
    idx = txt.find("func (i *int64Amount) AsInt64() (intValue int64, ok bool)")
    if idx == -1:
        idx = txt.find("func (i *int64Amount) AsInt64()")
    if idx == -1:
        continue
    lines = txt[idx:].splitlines()[:80]
    for line in lines:
        print(line)
py

python3 - <<'PY'
import subprocess
try:
    out = subprocess.check_output(["go", "env", "GOMODCACHE"], text=True).strip()
except Exception as e:
    print("GOMODCACHE unavailable:", e)
    raise SystemExit
print("GOMODCACHE=", out)
pats = [
    "pkg/api/resource/amount.go",
    "pkg/api/resource/quantity.go",
]
for path in pats:
    ps = subprocess.check_output(["find", out, "-path", f"*k8s.io/apimachinery-v0.36.3/{path}", "-type", "f"], text=True, stderr=subprocess.DEVNULL).strip().splitlines()
    print("===", path, len(ps), ps)
    if not ps:
        continue
    text = Path(ps[0]).read_text()
    for fn in ["func (i *int64Amount) AsInt64() (intValue int64, ok bool)", "func ValidateQuantity(q Quantity, fldPath *Path) *FieldError"]:
        idx = text.find(fn)
        if idx != -1:
            end = text.find("\nfunc ", idx+1)
            if end == -1:
                end = idx + 2500
            print("====", fn, "====")
            print(text[idx:end])
PY

echo "== module cache Go files =="
if [ -d "$HOME/go/pkg/mod/k8s.io" ]; then ls -R "$HOME/go/pkg/mod/k8s.io" | head -100; fi

Repository: Project-HAMi/HAMi

Length of output: 607

🌐 Web query:

k8s.io/apimachinery v0.36.3 resource Amount AsInt64 negative scale source

💡 Result:

In k8s.io/apimachinery (including version v0.36.3), the AsInt64() method on int64Amount is designed to return the amount as an int64 at scale 0 [1][2]. When dealing with a negative scale, the method explicitly returns 0, false [1][2]. The implementation follows this logic: 1. If the internal scale is 0, it returns the value and true [1][2]. 2. If the internal scale is negative (a.scale < 0), it returns 0, false [1][2]. This behavior reflects that a negative scale represents fractional components (e.g., milli, micro, nano), which cannot be represented as an int64 at scale 0 without potential loss of precision [1][2]. 3. If the internal scale is positive, it uses positiveScaleInt64 to calculate the result [1][2]. The AsInt64() method on the higher-level Quantity type similarly uses this underlying int64Amount logic [3][4]. It returns false if the Quantity is internally represented as a decimal (inf.Dec), or if the underlying int64Amount cannot perform the conversion (which includes cases where the scale is negative) [3][4][5]. In contexts where rounding occurs—such as in RoundUp or when converting scaled values—the library consistently enforces "rounding away from zero" (e.g., -9 at scale 1 rounds to -10) [3][6][4][7]. While the specific AsInt64() method simply fails (returns false) for negative scales, related rounding functions ensure that negative values are treated correctly by moving them further from zero [8][6][7].

Citations:

🌐 Web query:

Kubernetes Quantity negative scale valid as string "10000000.0" validation validation.go MilliValue 1000

💡 Result:

In Kubernetes, the Quantity type is a fixed-point representation used for resources like CPU and memory [1]. When parsing a string as a Quantity, the format allows for an optional sign ("+" or "-"), a mantissa (integer part), and optional exponents/suffixes [1]. Regarding your specific technical points: 1. Negative Scale in Quantity Logic: The internal implementation of Kubernetes quantities involves handling scales to represent fixed-point math accurately [2][1]. While string inputs can include negative numbers (e.g., "-100m"), a "negative scale" in the context of the underlying math library (like inf.Dec or resource.Scale) is an implementation detail for exponent representation [2][3]. Within the public API, "negative scale" is generally not a valid or exposed concept for users; the parser simply handles the numeric value provided [1]. Explicit validation functions, such as ValidateNonnegativeQuantity, are often used to enforce that specific resource fields remain positive, as negative values are logically invalid for most resource definitions [4]. 2. MilliValue and Validation: The MilliValue() function is an accessor that returns the value of a quantity in millicores (or equivalent units, i.e., value * 1000) [2][5]. It is implemented as q.ScaledValue(Milli), where Milli is defined with a scale of -3 [2]. Because MilliValue() performs an internal scaling operation, it can overflow an int64 if the underlying quantity is extremely large [2][5]. Kubernetes code often contains warnings that callers should verify the magnitude using Value() before calling MilliValue() if overflow is a concern, though there are ongoing discussions about improving this safety in the codebase [5]. 3. String Validation: When validating quantities in Kubernetes (e.g., via admission controllers or CEL), the standard practice is to use the native quantity(string) parsing logic, which rejects malformed strings [6][7][8]. String values like "10000000.0" are generally parsed as valid quantities, provided they do not exceed the internal architectural limits (magnitude constraints) imposed by the library, which rounds or caps values that are too large or too precise [1]. In summary, the Quantity library handles numeric strings, including fractional ones [1]. The "negative scale" mentioned is an internal mechanism for representing decimals and exponents [2], while MilliValue is a specific utility that requires caution regarding int64 overflow [5]. Validation logic for these fields typically relies on standard parsing or explicit constraints (like checking for non-negative values) to ensure data integrity [4][6].

Citations:

🌐 Web query:

kubernetes apimachinery v0.36.3 Int64Amount Scale negative AsInt64 code

💡 Result:

In Kubernetes apimachinery (including v0.36.x), the int64Amount type uses the AsInt64() method to attempt a fast conversion of a fixed-point value into an int64 at scale 0 [1][2]. When the scale of an int64Amount is negative, the AsInt64() method explicitly returns 0, false [1][2]: func (a int64Amount) AsInt64 (int64, bool) { if a.scale == 0 { return a.value, true } if a.scale < 0 { // TODO: attempt to reduce factors, although it is assumed that factors are reduced prior // to the int64Amount being created. return 0, false } return positiveScaleInt64(a.value, a.scale) } The code includes a TODO comment indicating that while the method currently returns false for negative scales, it could theoretically attempt to reduce factors; however, it assumes that factors are already reduced before the int64Amount is created [1][2]. Consequently, if a consumer encounters a negative scale, the method signals that the value cannot be represented as an int64 at scale 0 without potential loss of precision or simply because the fast-path optimization does not support that transformation [1][2]. If AsInt64() returns false, callers are generally expected to use the inf.Dec representation of the Quantity instead [3][4].

Citations:

Handle AsInt64() failures before falling back to percentage accounting.

Negative internal scales make AsInt64() return false, so whole-number decimal quantities such as 10000000.0 leave memnum == 0. This makes MemPercentagereq default to 100, causing Fit to use percentage accounting instead of the clamped request. Compare the Quantity with the intended bounds first, then convert only after the bounds check passes.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/device/cambricon/device.go` around lines 255 - 260, Update the
memory-request conversion logic surrounding memnums and memnum to compare the
Quantity against the intended lower and upper bounds before calling AsInt64().
If the request is out of range, clamp memnum to math.MaxInt32; otherwise convert
the validated quantity to the scaled integer, ensuring AsInt64() failures cannot
leave memnum at zero and trigger percentage accounting.

}
}
corenum := int32(100)
Expand Down
39 changes: 39 additions & 0 deletions pkg/device/cambricon/device_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package cambricon
import (
"context"
"flag"
"math"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -287,6 +288,44 @@ func Test_GenerateResourceRequests(t *testing.T) {
Coresreq: int32(2),
},
},
{
name: "vmemory expressed in Gi units no longer wraps to zero",
args: corev1.Container{
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
"cambricon.com/mlu": resource.MustParse("1"),
"cambricon.com/mlu.smlu.vmemory": resource.MustParse("16Gi"),
"cambricon.com/mlu.smlu.vcore": resource.MustParse("2"),
},
},
},
want: device.ContainerDeviceRequest{
Nums: int32(1),
Type: CambriconMLUDevice,
Memreq: int32(math.MaxInt32),
MemPercentagereq: int32(0),
Coresreq: int32(2),
},
},
{
name: "oversized plain vmemory value clamps to max int32",
args: corev1.Container{
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
"cambricon.com/mlu": resource.MustParse("1"),
"cambricon.com/mlu.smlu.vmemory": resource.MustParse("10000000"),
"cambricon.com/mlu.smlu.vcore": resource.MustParse("2"),
},
},
},
want: device.ContainerDeviceRequest{
Nums: int32(1),
Type: CambriconMLUDevice,
Memreq: int32(math.MaxInt32),
MemPercentagereq: int32(0),
Coresreq: int32(2),
},
},
Comment on lines +291 to +328

@coderabbitai coderabbitai Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add a regression case for negative memory values.

These cases cover 16Gi and an oversized positive value. They do not cover the negative-input branch described by this PR. Add a case with resource.MustParse("-1") for cambricon.com/mlu.smlu.vmemory. Assert Memreq == math.MaxInt32 and the existing Nums, Type, and Coresreq values. Otherwise, the negative clamp can regress without failing Test_GenerateResourceRequests.

As per PR objective, negative requests must be clamped to math.MaxInt32.

Suggested test case 
 		{
+			name: "negative vmemory value clamps to max int32",
+			args: corev1.Container{
+				Resources: corev1.ResourceRequirements{
+					Limits: corev1.ResourceList{
+						"cambricon.com/mlu":              resource.MustParse("1"),
+						"cambricon.com/mlu.smlu.vmemory": resource.MustParse("-1"),
+						"cambricon.com/mlu.smlu.vcore":   resource.MustParse("2"),
+					},
+				},
+			},
+			want: device.ContainerDeviceRequest{
+				Nums:             int32(1),
+				Type:             CambriconMLUDevice,
+				Memreq:           int32(math.MaxInt32),
+				MemPercentagereq: int32(0),
+				Coresreq:         int32(2),
+			},
+		},
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{
name: "vmemory expressed in Gi units no longer wraps to zero",
args: corev1.Container{
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
"cambricon.com/mlu": resource.MustParse("1"),
"cambricon.com/mlu.smlu.vmemory": resource.MustParse("16Gi"),
"cambricon.com/mlu.smlu.vcore": resource.MustParse("2"),
},
},
},
want: device.ContainerDeviceRequest{
Nums: int32(1),
Type: CambriconMLUDevice,
Memreq: int32(math.MaxInt32),
MemPercentagereq: int32(0),
Coresreq: int32(2),
},
},
{
name: "oversized plain vmemory value clamps to max int32",
args: corev1.Container{
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
"cambricon.com/mlu": resource.MustParse("1"),
"cambricon.com/mlu.smlu.vmemory": resource.MustParse("10000000"),
"cambricon.com/mlu.smlu.vcore": resource.MustParse("2"),
},
},
},
want: device.ContainerDeviceRequest{
Nums: int32(1),
Type: CambriconMLUDevice,
Memreq: int32(math.MaxInt32),
MemPercentagereq: int32(0),
Coresreq: int32(2),
},
},
{
name: "vmemory expressed in Gi units no longer wraps to zero",
args: corev1.Container{
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
"cambricon.com/mlu": resource.MustParse("1"),
"cambricon.com/mlu.smlu.vmemory": resource.MustParse("16Gi"),
"cambricon.com/mlu.smlu.vcore": resource.MustParse("2"),
},
},
},
want: device.ContainerDeviceRequest{
Nums: int32(1),
Type: CambriconMLUDevice,
Memreq: int32(math.MaxInt32),
MemPercentagereq: int32(0),
Coresreq: int32(2),
},
},
{
name: "negative vmemory value clamps to max int32",
args: corev1.Container{
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
"cambricon.com/mlu": resource.MustParse("1"),
"cambricon.com/mlu.smlu.vmemory": resource.MustParse("-1"),
"cambricon.com/mlu.smlu.vcore": resource.MustParse("2"),
},
},
},
want: device.ContainerDeviceRequest{
Nums: int32(1),
Type: CambriconMLUDevice,
Memreq: int32(math.MaxInt32),
MemPercentagereq: int32(0),
Coresreq: int32(2),
},
},
{
name: "oversized plain vmemory value clamps to max int32",
args: corev1.Container{
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
"cambricon.com/mlu": resource.MustParse("1"),
"cambricon.com/mlu.smlu.vmemory": resource.MustParse("10000000"),
"cambricon.com/mlu.smlu.vcore": resource.MustParse("2"),
},
},
},
want: device.ContainerDeviceRequest{
Nums: int32(1),
Type: CambriconMLUDevice,
Memreq: int32(math.MaxInt32),
MemPercentagereq: int32(0),
Coresreq: int32(2),
},
},
🧰 Tools
🪛 ast-grep (0.45.0)

[warning] 304-304: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(math.MaxInt32)
Note: [CWE-190] Integer Overflow or Wraparound.

(integer-overflow-narrowing-conversion-go)

[warning] 323-323: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(math.MaxInt32)
Note: [CWE-190] Integer Overflow or Wraparound.

(integer-overflow-narrowing-conversion-go)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/device/cambricon/device_test.go` around lines 291 - 328, Add a negative
vmemory regression case to the resource-request table using
resource.MustParse("-1") for cambricon.com/mlu.smlu.vmemory. In
Test_GenerateResourceRequests, assert Memreq is math.MaxInt32 while preserving
the existing Nums, Type, and Coresreq expectations.

}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
Expand Down
Loading