fix: prevent unsigned integer underflow in NVML memory info hook - #201
fix: prevent unsigned integer underflow in NVML memory info hook#201miaobyte wants to merge 4 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: miaobyte The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
34a7e14 to
4c26dce
Compare
4c26dce to
58d0f3c
Compare
58d0f3c to
668ecb8
Compare
miaobyte
changed the title
| switch (version) { | ||
| case 1: | ||
| ((nvmlMemory_t*)memory)->free = (limit-usage); | ||
| ((nvmlMemory_t*)memory)->free = limit - clamped; |
There was a problem hiding this comment.
This silently clamps the value without logging, so operators can't tell usage actually exceeded the limit.
There was a problem hiding this comment.
Agreed, a LOG_WARN here would help operators detect when usage exceeded the limit. Something like:
size_t clamped = (usage > limit) ? limit : usage;
if (usage > limit) {
LOG_WARN("NVML meminfo: usage %lu exceeds limit %lu, clamping", usage, limit);
}
|
This only fixes the NVML hook, not the CUDA hook underflow from the same issue, so closing #200 here would be premature. |
|
已收到，正在 review 你的 PR。 |
|
Thanks for the review @mesutoezdil. Agreed on both points:
@miaobyte please update when you have a chance. |
hami-robot
Bot
added
dco-signoff: no
size/S
and removed
dco-signoff: yes
size/XS
labels
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCUDA and NVML memory hooks now clamp usage exceeding configured limits, preserving bounded memory reporting instead of returning an error or underflowed free value. ChangesMemory Usage Clamping
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
b5007c1 to
b72a46f
Compare
b72a46f to
3c3d82f
Compare
3c3d82f to
c56e601
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/nvml/hook.c`:
- Around line 356-369: Update the NVML memory handling around the version switch
to derive actual_limit as the minimum of the configured limit and the original
NVML-reported total. Use actual_limit for usage clamping and for the free and
total fields in both nvmlMemory_t and nvmlMemory_v2_t, and warn when usage
exceeds actual_limit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 44929024-f7e6-470f-8905-80b61a875ce9
📒 Files selected for processing (2)
src/cuda/memory.csrc/nvml/hook.c
|
done,now updated: NVML hook (
CUDA hook (
Both hooks now handle |
|
收到，我们先测试一下。 |
|
测试通过，8x RTX 3090 上编译和运行均正常。@archlitchi 可以 merge 了。 |
|
@miaobyte Thanks for the updates, LOG_WARN and the CUDA fix look good. One item from CodeRabbit's review is still open though: on the NVML side, Can you apply the same min() clamp on the NVML side before we merge? |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/nvml/hook.c (1)
356-374:⚠️ Potential issue | 🟠 MajorClamp against
actual_limit, notlimit.
clampedis computed before each version-specificactual_limit. Iflimitexceeds physical memory andusagefalls between them, the hook can reporttotal=8 GiB,free=0, andused=10 GiB, violatingfree + used = total; the warning is also skipped. Computeactual_limitfirst and use it for both the warning condition and usage clamp in both API versions.🤖 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 `@src/nvml/hook.c` around lines 356 - 374, Update the version-specific handling in the NVML meminfo switch for both cases 1 and 2: compute actual_limit from limit and physical_total before clamping usage, then use actual_limit for the usage clamp and warning condition. Remove the shared pre-switch clamped calculation so reported free, used, and total remain consistent and warnings cover usage above the effective limit.
🤖 Prompt for all review comments with 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.
Duplicate comments:
In `@src/nvml/hook.c`:
- Around line 356-374: Update the version-specific handling in the NVML meminfo
switch for both cases 1 and 2: compute actual_limit from limit and
physical_total before clamping usage, then use actual_limit for the usage clamp
and warning condition. Remove the shared pre-switch clamped calculation so
reported free, used, and total remain consistent and warnings cover usage above
the effective limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3900b0df-c9b5-4a33-aa81-4aee1967404a
📒 Files selected for processing (2)
src/cuda/memory.csrc/nvml/hook.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cuda/memory.c
When usage > limit, (limit - usage) wraps around to a massive unsigned value (~18 EB). Clamp both free and used so that free + used = total always holds, preventing corrupted values in monitoring tools. Fixes Project-HAMi#200 Signed-off-by: peng.li24 <peng.li24@nio.com> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: peng.li24 <734991033@qq.com>
cuMemGetInfo_v2: usage 超过 limit 时不再返回 INVALID_VALUE， 而是 clamp 到 limit 再计算 free memory。 nvmlDeviceGetMemoryInfo: 同样添加 clamp + 警告日志。 Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: peng.li24 <734991033@qq.com> Signed-off-by: miaobyte <lipeng@mirrorsoft.cn> Signed-off-by: peng.li24 <734991033@qq.com>
…sical_total) Match the CUDA path behavior in src/cuda/memory.c:520 — if the configured limit exceeds the real GPU memory, report the physical total rather than the impossibly-high limit. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: peng.li24 <734991033@qq.com>
363599e to
fee12be
Compare
|
pls remove claude commits from history |
|
and pls pls refrain from force pushing once you have started getting reviews |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/nvml/hook.c (1)
356-374: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClamp usage against
actual_limit, not the configuredlimit.
clampedis computed before each case derivesactual_limit. Iflimitexceeds physical memory andusageis between the physical total andlimit, the hook reportsused > totalandfree + used != total. Move the effective-limit and clamp calculation before the version switch, and warn whenusage > actual_limitfor both API versions.Proposed fix
- size_t clamped = (usage > limit) ? limit : usage; - if (usage > limit) { - LOG_WARN("NVML meminfo: usage %lu exceeds limit %lu, clamping", usage, limit); - } + size_t physical_total = version == 1 + ? ((nvmlMemory_t *)memory)->total + : ((nvmlMemory_v2_t *)memory)->total; + size_t actual_limit = (limit > physical_total) ? physical_total : limit; + size_t clamped = (usage > actual_limit) ? actual_limit : usage; + if (usage > actual_limit) { + LOG_WARN("NVML meminfo: usage %zu exceeds limit %zu, clamping", + usage, actual_limit); + } switch (version) { case 1: { - size_t physical_total = ((nvmlMemory_t*)memory)->total; - size_t actual_limit = (limit > physical_total) ? physical_total : limit; ... } case 2: { - size_t physical_total = ((nvmlMemory_v2_t *)memory)->total; - size_t actual_limit = (limit > physical_total) ? physical_total : limit; ... }🤖 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 `@src/nvml/hook.c` around lines 356 - 374, Move effective-limit calculation ahead of the version switch, deriving it from each version’s physical total, then clamp usage against that effective limit. Update the warning to trigger and report when usage exceeds actual_limit, and ensure both version 1 and version 2 populate free, total, and used from the same consistent clamped values.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/nvml/hook.c`:
- Around line 357-358: Update the LOG_WARN call in the usage-versus-limit check
to use %zu for both size_t meminfo arguments, usage and limit, matching their
types while preserving the existing warning message and clamping behavior.
---
Duplicate comments:
In `@src/nvml/hook.c`:
- Around line 356-374: Move effective-limit calculation ahead of the version
switch, deriving it from each version’s physical total, then clamp usage against
that effective limit. Update the warning to trigger and report when usage
exceeds actual_limit, and ensure both version 1 and version 2 populate free,
total, and used from the same consistent clamped values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 14ad3b5c-c7c7-4454-9280-a69039dbb824
📒 Files selected for processing (2)
src/cuda/memory.csrc/nvml/hook.c
| if (usage > limit) { | ||
| LOG_WARN("NVML meminfo: usage %lu exceeds limit %lu, clamping", usage, limit); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
rg -n 'LOG_WARN\("NVML meminfo' src/nvml/hook.c
rg -n '%z[dux]' src/nvml/hook.cRepository: Project-HAMi/HAMi-core
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -eu
echo "== files named hook.c under src/nvml =="
fd '^hook\.c$' src || true
echo "== definition of LOG_WARN / related macros =="
rg -n 'define\s+LOG_WARN|typedef|NVML|NVML_.*meminfo|nvml' src include . -g '!**/.git/**' | head -n 200
echo "== relevant hook.c lines =="
sed -n '1,430p' src/nvml/hook.c | nl -ba | sed -n '1,430p' > /tmp/cur_hook_snip.txt
sed -n '330,375p' src/nvml/hook.c | nl -ba
echo "== include files in hook.c =="
sed -n '1,80p' src/nvml/hook.cRepository: Project-HAMi/HAMi-core
Length of output: 13485
🏁 Script executed:
#!/bin/bash
set -eu
tmp="$(mktemp)"
echo "== src/nvml/hook.c first 90 lines =="
sed -n '1,90p' src/nvml/hook.c
echo "== lines with LOG_WARN definition/usages =="
sed -n '1,600p' src/nvml/hook.c | grep -nE 'LOG_WARN|LOG_|define\s+LOG' > "$tmp" || true
if [ -s "$tmp" ]; then cat "$tmp"; fi
echo "== relevant hook.c lines around target =="
sed -n '340,365p' src/nvml/hook.c
echo "== check available compilers/configs for platform assumptions =="
for f in CMakeLists.txt Makefile configure.ac src/*.h.in src/nvml/*.h src/nvml/*.c; do
[ -e "$f" ] && {
echo "-- $f --"
grep -nE 'size_t|%lu|%zu|LOG_WARN|printf|format|inttypes|stdint|GCC|clang|MESON_C_FLAGS|CFLAGS|WFORMAT|Wvla|NVML' "$f" | head -n 50 || true
}
doneRepository: Project-HAMi/HAMi-core
Length of output: 14441
🏁 Script executed:
#!/bin/bash
set -eu
echo "== include/libnvml_hook.h =="
sed -n '1,220p' include/libnvml_hook.h
echo "== include/utils.h LOG macro definitions =="
sed -n '1,220p' include/utils.h
echo "== deterministic model of %lu vs size_t by compiler/platform header (non-repository probe) =="
tmp="$(mktemp .probeXXXX.c)"
cat > "$tmp" <<'EOF'
`#include` <stddef.h>
`#include` <stdio.h>
int main(void) {
printf("size_t=%zu\n", sizeof(size_t));
size_t s = 42;
printf("%lu\n", s);
printf("%zu\n", s);
return 0;
}
EOF
if command -v cc >/dev/null 2>&1; then
cc "$tmp" -o "${tmp}.out" 2>&1 || true
"${tmp}.out" 2>&1 || true
else
echo "cc not available"
fi
rm -f "$tmp" "${tmp}.out"Repository: Project-HAMi/HAMi-core
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -eu
echo "== repository root =="
pwd
echo "== locate HEADER/UTL files =="
git ls-files | grep -E '(^|/)(libnvml_hook\.h|utils\.h)$' || true
echo "== find header-like candidates in first lines =="
for f in $(git ls-files | grep -E '\.(h|hpp)$' || true); do
case "$f" in
*include*utils.h|*utils.h|*libnvml_hook.h)
echo "-- $f --"
sed -n '1,180p' "$f"
;;
esac
done
echo "== current hook target with line numbers =="
awk 'NR>=330 && NR<=365 { printf "%5d\t%s\n", NR, $0 }' src/nvml/hook.cRepository: Project-HAMi/HAMi-core
Length of output: 17751
Use %zu for the size_t meminfo fields.
LOG_WARN forwards variadic arguments to fprintf, so %lu only matches when size_t is unsigned long; this is undefined behavior on targets where size_t is unsigned int. Replace the two %lu placeholders at src/nvml/hook.c:358 with %zu.
🧰 Tools
🪛 Cppcheck (2.21.0)
[warning] 358-358: If resource allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfResources)
🤖 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 `@src/nvml/hook.c` around lines 357 - 358, Update the LOG_WARN call in the
usage-versus-limit check to use %zu for both size_t meminfo arguments, usage and
limit, matching their types while preserving the existing warning message and
clamping behavior.
Compute physical_total and actual_limit first, then derive clamped and the warning from actual_limit instead of the raw limit. Prevents broken free+used=total invariant when limit exceeds physical GPU memory and usage falls between them. Signed-off-by: peng.li24 <734991033@qq.com>
3 participants
Summary
Fix unsigned integer underflow in
_nvmlDeviceGetMemoryInfo: when GPU memory usage exceeds the configured limit,(limit - usage)with unsignedsize_twraps to ~18 EB, breaking monitoring tools.Changes
src/nvml/hook.c: computeclamped = (usage > limit) ? limit : usage, then derive bothused = clampedandfree = limit - clamped. This guaranteesfree + used = totalalways holds andfreenever underflows.Fixes #200
🤖 Generated with Claude Code
Summary by CodeRabbit