google-deepmind / google-deepmind/mujoco
mj_recompile reads old joint state using the new joint-type width
- 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
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