google-deepmind / google-deepmind/mujoco

mj_recompile reads old joint state using the new joint-type width

Open
#3,586 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
C++
Stars
15.2k
Forks
1.8k
Avg merge
10d 16h
Merged PRs (30d)
25

Description

### Intro

Hi!

I am testing state continuity across live `mjSpec` schema changes. Changing an existing joint from hinge to free before `mj_recompile` makes state preservation read the old `mjData` with the new joint width.

### My setup

- MuJoCo source build from `main` at `10124d5d9dca411ec3c8988aa1e3b619103d71bb`
- C API, double precision
- Ubuntu 24.04.5 LTS, Linux 7.0.0-28-generic x86_64
- Normal GCC 11.5.0 build and GCC 13.3.0 AddressSanitizer build

### What's happening? What did you expect?

After changing a joint type, I expected `mj_recompile` either to initialize the incompatible new coordinate schema from the new model defaults, perform a documented conversion, or reject the transition. It should not read beyond the old joint state.

`SaveState` uses the joint's mutated `spec.type` to obtain `nq=7` and `nv=6`, while `qposadr_` and `dofadr_` still address the old one-element hinge state. The normal build silently restores the hinge angle as a free-joint translation and creates an invalid zero quaternion before forward processing:

```text
result=0 nq=7 nv=6 qpos=0.25,0,0,0,0,0,0 qvel=0.5,0,0,0,0,0
after forward xpos=0.25,0,0 xquat=1,0,0,0 qacc=0,0,0,0,0,0
```

An AddressSanitizer build reports:

```text
ERROR: AddressSanitizer: use-after-poison
READ of size 56
#7 mjCModel::SaveState ... src/user/user_model.cc:4090
#8 mj_recompile ... src/user/user_api.cc:332
```

Relevant source:

- https://github.com/google-deepmind/mujoco/blob/10124d5d9dca411ec3c8988aa1e3b619103d71bb/src/user/user_api.cc#L324-L350
- https://github.com/google-deepmind/mujoco/blob/10124d5d9dca411ec3c8988aa1e3b619103d71bb/src/user/user_model.cc#L4076-L4095
- https://github.com/google-deepmind/mujoco/blob/10124d5d9dca411ec3c8988aa1e3b619103d71bb/src/user/user_objects.h#L728-L742

### Steps for reproduction

1. Build the commit above normally and with AddressSanitizer.
2. Compile `poc/reproducer.cc` against each build using `poc/README.md`.
3. Run the normal binary and observe the physically misinterpreted state.
4. Run the sanitized binary and observe the 56-byte invalid read in `SaveState`.

Reproduction package:

[recompile-joint-type-width-overread-poc.zip](https://github.com/user-attachments/files/32221302/recompile-joint-type-width-overread-poc.zip)

### Minimal model for reproduction

The complete asset-free MJCF is embedded in the reproducer. It has one body, one hinge, and one sphere.

### Code required for reproduction

See the attached `poc/reproducer.cc`. A safe production fix should retain the old compiled schema while saving state and define a policy for incompatible joint-type transitions. I have included design notes rather than a speculative patch.

### Confirmations

- [x] I searched the [latest documentation](https://mujoco.readthedocs.io/en/latest/overview.html) thoroughly before posting.
- [x] I searched previous [Issues](https://github.com/google-deepmind/mujoco/issues) and [Discussions](https://github.com/google-deepmind/mujoco/discussions), I am certain this has not been raised before.

Contributor guide

Open the contributing guide

Research direction

Start with src/user/user_api.cc around mj_recompile and src/user/user_model.cc around SaveState, then build and run poc/reproducer.cc using poc/README.md. Compare normal and AddressSanitizer runs, and use the reproduction to verify that joint-type changes no longer read the old state with the new width and that the incompatible-transition policy is enforced.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend-api-design
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.