google-deepmind / google-deepmind/mujoco

mj_recompile reads past saved actuator state when an actuator's actdim increases

Open
#3,590 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. I first sent this finding to Google's vulnerability intake, and they advised reporting it as an issue in the MuJoCo repository.

### My setup

- MuJoCo 3.13.0 at `123347c0eeab7e13c8da0828ab593bbd95bcf335`
- MuJoCo `main` at `10124d5d9dca411ec3c8988aa1e3b619103d71bb`
- C API from a minimal C++17 client, double precision
- Ubuntu 24.04.5 LTS, Linux 7.0.0-28-generic x86_64
- GCC / G++ 13.3.0
- Normal build and whole-library AddressSanitizer build

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

Expanding a surviving actuator's activation dimension before calling `mj_recompile` makes state restoration read past the end of the saved activation vector.

The reproducer initially compiles a general actuator with `dyntype="integrator"`, giving it one activation value. It then changes the same actuator to `dyntype=mjDYN_USER`, sets `actdim=3`, and calls `mj_recompile`.

I expected recompilation to do one of the following safely: reject the incompatible transition, reset the actuator state, or preserve only the compatible old component and initialize the two added components. It should not read beyond the state that was saved.

Instead, `SaveState` stores one `mjtNum` using the old `actdim_`. Compilation changes `actdim_` to 3, and `RestoreState` then passes that new dimension to `mjuu_copyvec` while reading from the one-element saved vector. AddressSanitizer reports a 24-byte read immediately after an 8-byte allocation.

On release 3.13.0:

```text
ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 24
#7 mjCModel::RestoreState ... src/user/user_model.cc:4147
#8 mj_recompile ... src/user/user_api.cc:340
0x5020000013f8 is located 0 bytes after 8-byte region
```

Current `main` produces the same result at `user_model.cc:4154`. An uninstrumented build returned success and printed the following on my validation host:

```text
result=0 na=3 act=42,0,0
```

The two trailing values are allocator-dependent; this output is not the correctness oracle. The sanitizer report is the deterministic evidence of the invalid source read.

Relevant source on release 3.13.0:

- [`actdim_` is replaced during compilation](https://github.com/google-deepmind/mujoco/blob/123347c0eeab7e13c8da0828ab593bbd95bcf335/src/user/user_model.cc#L3838-L3842)
- [`SaveState` sizes the saved vector with the old activation dimension](https://github.com/google-deepmind/mujoco/blob/123347c0eeab7e13c8da0828ab593bbd95bcf335/src/user/user_model.cc#L4090-L4095)
- [`RestoreState` copies using the new activation dimension](https://github.com/google-deepmind/mujoco/blob/123347c0eeab7e13c8da0828ab593bbd95bcf335/src/user/user_model.cc#L4143-L4148)

The corresponding current-`main` save and restore locations are [`user_model.cc:4097-4102`](https://github.com/google-deepmind/mujoco/blob/10124d5d9dca411ec3c8988aa1e3b619103d71bb/src/user/user_model.cc#L4097-L4102) and [`user_model.cc:4150-4155`](https://github.com/google-deepmind/mujoco/blob/10124d5d9dca411ec3c8988aa1e3b619103d71bb/src/user/user_model.cc#L4150-L4155).

This is related to, but distinct from, #3586. That issue reads the old `qpos` / `qvel` arrays during `SaveState` using a mutated joint width. This issue saves the old actuator activation width, then reads past that saved vector during `RestoreState` using the newly compiled `actdim_`.

### Steps for reproduction

1. Check out MuJoCo 3.13.0 at `123347c0eeab7e13c8da0828ab593bbd95bcf335`.
2. Save the code below as `/tmp/recompile-actdim-reproducer.cc`.
3. From the MuJoCo checkout, run:

```bash
cmake -S . -B build-actdim-asan \
-DCMAKE_BUILD_TYPE=Debug \
-DCMAKE_C_COMPILER=gcc-13 \
-DCMAKE_CXX_COMPILER=g++-13 \
-DCMAKE_C_FLAGS='-fsanitize=address -fno-omit-frame-pointer' \
-DCMAKE_CXX_FLAGS='-fsanitize=address -fno-omit-frame-pointer' \
-DCMAKE_SHARED_LINKER_FLAGS='-fsanitize=address' \
-DMUJOCO_BUILD_EXAMPLES=OFF \
-DMUJOCO_BUILD_SIMULATE=OFF \
-DMUJOCO_BUILD_TESTS=OFF
cmake --build build-actdim-asan --target mujoco --parallel
g++-13 -std=c++17 -O0 -g -fsanitize=address -fno-omit-frame-pointer \
-Iinclude /tmp/recompile-actdim-reproducer.cc \
-Lbuild-actdim-asan/lib -Wl,-rpath,'$ORIGIN/lib' \
-lmujoco -o build-actdim-asan/reproducer
ASAN_OPTIONS=detect_leaks=0:halt_on_error=1 \
build-actdim-asan/reproducer
```

4. Observe the 24-byte heap-buffer-overflow read in `mjCModel::RestoreState`.

The attached [recompile-actdim-heap-overread-poc.zip](https://github.com/user-attachments/files/32224317/recompile-actdim-heap-overread-poc.zip) contains the same source, commands, environment information, and complete sanitizer traces for 3.13.0 and current `main`.

### Minimal model for reproduction

The complete asset-free MJCF is embedded in the program below. It contains one body, one hinge joint, one sphere, and one actuator.

### Code required for reproduction

```cpp
#include
#include

#include

int main() {
static constexpr char xml[] = R"(










)";

char error[1024] = {};
mjSpec* spec = mj_parseXMLString(xml, nullptr, error, sizeof(error));
if (!spec) {
std::fprintf(stderr, "parse failed: %s\n", error);
return EXIT_FAILURE;
}
mjModel* model = mj_compile(spec, nullptr);
mjData* data = model ? mj_makeData(model) : nullptr;
if (!model || !data) {
std::fprintf(stderr, "initial compile failed: %s\n", mjs_getError(spec));
return EXIT_FAILURE;
}

data->act[0] = 42.0;
mjsActuator* actuator =
mjs_asActuator(mjs_findElement(spec, mjOBJ_ACTUATOR, "actuator"));
actuator->dyntype = mjDYN_USER;
actuator->actdim = 3;

const int result = mj_recompile(spec, nullptr, model, data);
std::printf("result=%d na=%td act=", result, model->na);
for (int i = 0; i < model->na; ++i) {
std::printf("%s%.17g", i ? "," : "", data->act[i]);
}
std::printf("\n");

mj_deleteData(data);
mj_deleteModel(model);
mj_deleteSpec(spec);
return result == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
}
```

I have not included a speculative production patch because changing both activation width and dynamics type may require resetting the actuator state rather than copying a prefix. A safe implementation must retain the saved cardinality, never copy beyond it, and define the compatibility policy for changed activation semantics.

### 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 in src/user/user_model.cc at SaveState and RestoreState, using the linked current-main locations. Build the ASAN configuration and run the provided recompile-actdim reproducer to confirm the invalid read. Done means mj_recompile handles the changed actuator activation dimension without reading beyond saved state and the reproducer completes cleanly with tests covering the transition.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
robotics
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.