Skip to content

fix: harden strcat overflow and fwrite error handling in model save#3194

Open
tombudd wants to merge 1 commit intogoogle-deepmind:mainfrom
tombudd:una/fix-c-safety-hardening-v2
Open

fix: harden strcat overflow and fwrite error handling in model save#3194
tombudd wants to merge 1 commit intogoogle-deepmind:mainfrom
tombudd:una/fix-c-safety-hardening-v2

Conversation

@tombudd
Copy link
Copy Markdown

@tombudd tombudd commented Mar 24, 2026

Summary

Two minimal C safety hardening fixes found by UNA (autonomous security auditor, designed and built by Tom Buddtom@tombudd.com):

1. Buffer overflow in ui_main.cstrcatstrncat

File: src/ui/ui_main.c:1897

shortcuthelp() concatenates a modifier string and a key name into a 50-byte stack buffer using bare strcat(text, key). If key is sufficiently long, this overflows text[50].

Fix: Replace with strncat(text, key, sizeof(text) - strlen(text) - 1) — consistent with MuJoCo's own strncat usage at lines 1007/1009 and with the project's prior safer-string-handling work (changelog: "replaced strcat, strcpy, and sprintf with strncat, strncpy, and snprintf respectively"). This instance was missed in that sweep.

2. Silent data corruption in engine_io.c — unchecked fwrite errors

File: src/engine/engine_io.cmj_saveModel()

mj_saveModel() writes the entire model via multiple fwrite() calls, then calls fclose(fp) without checking for write errors. If any write fails (disk full, I/O error, NFS timeout), the function returns silently, leaving a truncated/corrupt .mjb file with no indication of failure.

Fix: Capture ferror(fp) state before fclose() (necessary because fclose may clear error state), then also check fclose() return value (which can fail on buffered flush). Emit mju_warning() if either indicates failure — using MuJoCo's own warning mechanism, consistent with surrounding code.

Changes

File Change Lines
src/ui/ui_main.c strcatstrncat with bounds +1 / -1
src/engine/engine_io.c Add ferror + fclose error check +4 / -1

Total: 2 files, +5 / -2 lines

Why these fixes matter

  • Both follow existing MuJoCo conventions — no new patterns or dependencies introduced
  • Both are success-path-transparent: zero behavioral change when nothing goes wrong
  • The strcat fix closes a stack buffer overflow (potential code execution in adversarial scenarios)
  • The fwrite/fclose fix prevents silent model corruption that could cause hard-to-diagnose simulation failures downstream — particularly relevant in safety-critical robotics contexts

About This Review

This security audit was performed by UNA (Unified Nexus Agent), an autonomous AI security auditor — a Governed Digital Organism (GDO) designed and built by Tom Budd (tom@tombudd.com | tombudd.com).

Note: Google Individual CLA previously signed by tom@tombudd.com (Mar 23, 2026) — cla/google check should pass.

Two C safety fixes identified by automated security audit:

1. src/ui/ui_main.c: Replace bare strcat(text, key) with
   strncat(text, key, sizeof(text) - strlen(text) - 1) to prevent
   buffer overflow on the 50-byte stack buffer in shortcuthelp().
   Consistent with MuJoCo's existing strncat usage at lines 1007/1009
   and with the project's prior safer-string-handling work noted in
   the changelog.

2. src/engine/engine_io.c: Capture ferror(fp) state before fclose()
   in mj_saveModel(), then also check fclose() return value. Without
   this, I/O errors during model serialization (disk full, permission
   denied, NFS timeout) are silently swallowed, producing corrupt .mjb
   files with no warning to the caller. Checking both ferror and fclose
   is necessary: ferror must be read before fclose (which may clear the
   error state), and fclose itself can fail on buffered writes.

Both fixes follow existing MuJoCo conventions and introduce no
behavioral changes on success paths.

Reviewed-by: UNA-GDO sovereign-v2.0 (Autonomous Security Auditor)
Built-by: Tom Budd <tom@tombudd.com> — tombudd.com
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