Skip to content

script: Fix buffer over and under flows in script_buftoenv - #668

Merged
rsmarples merged 3 commits into
masterfrom
script
Jun 22, 2026
Merged

script: Fix buffer over and under flows in script_buftoenv#668
rsmarples merged 3 commits into
masterfrom
script

Conversation

@rsmarples

Copy link
Copy Markdown
Member

Enforce we have a buffer and it is terminated.
Rework code so that we no longer assert.

Reported by NVIDIA Project Vanessa

Enforce we have a buffer and it is terminated.
Rework code so that we no longer assert.
@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e350b505-aa55-4660-acd3-83916b40b1a1

📥 Commits

Reviewing files that changed from the base of the PR and between e8a17b9 and 122e908.

📒 Files selected for processing (1)
  • src/script.c
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/script.c

Walkthrough

script_buftoenv() in src/script.c is hardened: it now rejects zero-length buffers, validates NUL termination of the input buffer, counts strings by skipping consecutive NUL bytes, and reworks envp pointer array construction to tolerate leading empty strings and consecutive NUL separators. The assert.h include is removed.

Changes

script_buftoenv() Hardening

Layer / File(s) Summary
Buffer validation and NUL-tolerant string counting
src/script.c
Removes assert.h. Adds early return for zero-length buffer. Validates that the last byte is NUL (sets EINVAL and returns NULL if not). Replaces strict string-count loop with one that skips consecutive NUL bytes.
envp pointer array construction rework
src/script.c
Replaces the prior pointer-advancement and "avoid last terminator" strategy with a loop that optionally skips leading empty strings when the buffer begins with NUL, then captures one pointer per variable while skipping consecutive NUL separators.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main purpose of the PR: fixing buffer over and underflows in the script_buftoenv function.
Description check ✅ Passed The description is directly related to the changeset, explaining the enforcement of buffer validation and the elimination of assertions, which aligns with the summary of changes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch script

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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/script.c`:
- Around line 167-171: The code in script_buftoenv() checks if len is zero but
does not validate that buf is not NULL before dereferencing it at buf[len - 1].
Add a NULL pointer check for buf before accessing buf[len - 1] to prevent
crashes when NULL is passed with a non-zero length. Additionally, set errno when
returning NULL in the zero-length case to avoid callers encountering stale error
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

Run ID: 7913e605-9ad1-4ef0-b7f1-c95c6d84c279

📥 Commits

Reviewing files that changed from the base of the PR and between 7891a22 and e8a17b9.

📒 Files selected for processing (1)
  • src/script.c

Comment thread src/script.c Outdated
@rsmarples
rsmarples merged commit 7240d83 into master Jun 22, 2026
9 checks passed
@rsmarples
rsmarples deleted the script branch June 22, 2026 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant