chenlin/Mujoco_WASM
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Check mjs_getName return value in XML parser. Fixes #2898.

Browse Source 
PiperOrigin-RevId: 819351410
Change-Id: I0807d71328068f6388991ae15274c7d61b4d3e53
This commit is contained in:
Alessio Quaglino
2025-10-14 12:38:51 -07:00
committed by Copybara-Service
parent 67960543a0
commit b6f25ca623
11 changed files with 152 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files
cmake/MujocoLinkOptions.cmake
+2 -1
View File
@@ -18,11 +18,12 @@ include(CheckCSourceCompiles)
# linker.
function(get_mujoco_extra_link_options OUTPUT_VAR)
if(MSVC)
set(EXTRA_LINK_OPTIONS /OPT:REF /OPT:ICF=5)
set(EXTRA_LINK_OPTIONS /OPT:REF /OPT:ICF=5 /STACK:16777216)
else()
set(EXTRA_LINK_OPTIONS)
if(WIN32)
set(EXTRA_LINK_OPTIONS ${EXTRA_LINK_OPTIONS} -Wl,/STACK:16777216)
set(CMAKE_REQUIRED_FLAGS "-fuse-ld=lld-link")
check_c_source_compiles("int main() {}" SUPPORTS_LLD)
if(SUPPORTS_LLD)
doc/changelog.rst
+9
View File
@@ -2,6 +2,15 @@
Changelog
=========
Upcoming version (not yet released)
-----------------------------------
General
^^^^^^^^^
- Raise an error if there are name collisions also during parsing.
- Increase Windows stack size to 16MB to enable models with deep nested body hierarchies.
Version 3.3.7 (October 13, 2025)
-----------------------------------
sample/cmake/MujocoLinkOptions.cmake
+2 -1
View File
@@ -18,11 +18,12 @@ include(CheckCSourceCompiles)
# linker.
function(get_mujoco_extra_link_options OUTPUT_VAR)
if(MSVC)
set(EXTRA_LINK_OPTIONS /OPT:REF /OPT:ICF=5)
set(EXTRA_LINK_OPTIONS /OPT:REF /OPT:ICF=5 /STACK:16777216)
else()
set(EXTRA_LINK_OPTIONS)
if(WIN32)
set(EXTRA_LINK_OPTIONS ${EXTRA_LINK_OPTIONS} -Wl,/STACK:16777216)
set(CMAKE_REQUIRED_FLAGS "-fuse-ld=lld-link")
check_c_source_compiles("int main() {}" SUPPORTS_LLD)
if(SUPPORTS_LLD)
simulate/cmake/MujocoLinkOptions.cmake
+2 -1
View File
@@ -18,11 +18,12 @@ include(CheckCSourceCompiles)
# linker.
function(get_mujoco_extra_link_options OUTPUT_VAR)
if(MSVC)
set(EXTRA_LINK_OPTIONS /OPT:REF /OPT:ICF=5)
set(EXTRA_LINK_OPTIONS /OPT:REF /OPT:ICF=5 /STACK:16777216)
else()
set(EXTRA_LINK_OPTIONS)
if(WIN32)
set(EXTRA_LINK_OPTIONS ${EXTRA_LINK_OPTIONS} -Wl,/STACK:16777216)
set(CMAKE_REQUIRED_FLAGS "-fuse-ld=lld-link")
check_c_source_compiles("int main() {}" SUPPORTS_LLD)
if(SUPPORTS_LLD)
src/user/user_api.cc
+4
View File
@@ -279,6 +279,10 @@ mjsElement* mjs_attach(mjsElement* parent, const mjsElement* child,
// get error message from model
const char* mjs_getError(mjSpec* s) {
if (!s) {
mju_error("spec is null");
return nullptr;
}
mjCModel* modelC = static_cast<mjCModel*>(s->element);
return modelC->GetError().message;
}
src/user/user_model.cc
+50 -29
View File
@@ -27,6 +27,7 @@
#include <filesystem> // NOLINT(build/c++17)
#include <functional>
#include <mutex>
#include <sstream>
#include <string>
#include <string_view>
#include <thread>
@@ -4999,78 +5000,98 @@ void mjCModel::TryCompile(mjModel*& m, mjData*& d, const mjVFS* vfs) {
std::string mjCModel::PrintTree(const mjCBody* body, std::string indent) {
std::string tree;
tree += indent + "<body>\n";
indent += " ";
static void PrintIndent(std::stringstream& ss, int depth) {
// A static string of spaces, created only once during the program's lifetime.
static const std::string spaces(1024, ' ');
if (depth > 0) {
// Write 'depth * 2' spaces directly to the stringstream
// without creating any new std::string objects.
ss.write(spaces.c_str(), std::min((size_t)depth * 2, spaces.length()));
}
}
void mjCModel::PrintTree(std::stringstream& tree, const mjCBody* body, int depth) {
if (depth == 1024) {
throw mjCError(body, "depth limit exceeded in signature computation");
}
PrintIndent(tree, depth);
tree << "<body>\n";
for (const auto& joint : body->joints) {
tree += indent + "<joint>" + std::to_string(joint->nq()) + "</joint>\n";
PrintIndent(tree, depth + 1);
tree << "<joint>" << std::to_string(joint->nq()) << "</joint>\n";
}
for (uint64_t i = 0; i < body->geoms.size(); ++i) {
tree += indent + "<geom/>\n";
PrintIndent(tree, depth + 1);
tree << "<geom/>\n";
}
for (uint64_t i = 0; i < body->sites.size(); ++i) {
tree += indent + "<site/>\n";
PrintIndent(tree, depth + 1);
tree << "<site/>\n";
}
for (uint64_t i = 0; i < body->cameras.size(); ++i) {
tree += indent + "<camera/>\n";
PrintIndent(tree, depth + 1);
tree << "<camera/>\n";
}
for (uint64_t i = 0; i < body->lights.size(); ++i) {
tree += indent + "<light/>\n";
PrintIndent(tree, depth + 1);
tree << "<light/>\n";
}
for (uint64_t i = 0; i < body->bodies.size(); ++i) {
tree += PrintTree(body->bodies[i], indent);
PrintTree(tree, body->bodies[i], depth + 1);
}
indent.pop_back();
indent.pop_back();
tree += indent + "</body>\n";
return tree;
PrintIndent(tree, depth);
tree << "</body>\n";
}
uint64_t mjCModel::Signature() {
std::string tree = "\n" + PrintTree(bodies_[0]);
std::stringstream tree;
tree << "\n";
PrintTree(tree, bodies_[0]);
for (unsigned int i = 0; i < flexes_.size(); ++i) {
tree += "<flex/>\n";
tree << "<flex/>\n";
}
for (unsigned int i = 0; i < meshes_.size(); ++i) {
tree += "<mesh/>\n";
tree << "<mesh/>\n";
}
for (unsigned int i = 0; i < skins_.size(); ++i) {
tree += "<skin/>\n";
tree << "<skin/>\n";
}
for (unsigned int i = 0; i < hfields_.size(); ++i) {
tree += "<heightfield/>\n";
tree << "<heightfield/>\n";
}
for (unsigned int i = 0; i < textures_.size(); ++i) {
tree += "<texture/>\n";
tree << "<texture/>\n";
}
for (unsigned int i = 0; i < materials_.size(); ++i) {
tree += "<material/>\n";
tree << "<material/>\n";
}
for (unsigned int i = 0; i < pairs_.size(); ++i) {
tree += "<pair/>\n";
tree << "<pair/>\n";
}
for (unsigned int i = 0; i < excludes_.size(); ++i) {
tree += "<exclude/>\n";
tree << "<exclude/>\n";
}
for (unsigned int i = 1; i < equalities_.size(); ++i) {
tree += "<equality/>\n";
tree << "<equality/>\n";
}
for (unsigned int i = 0; i < tendons_.size(); ++i) {
tree += "<tendon/>\n";
tree << "<tendon/>\n";
}
for (unsigned int i = 0; i < actuators_.size(); ++i) {
tree += "<actuator/>\n";
tree << "<actuator/>\n";
}
for (unsigned int i = 0; i < sensors_.size(); ++i) {
tree += "<sensor>" + std::to_string(sensors_[i]->spec.type) + "<sensor/>\n";
tree << "<sensor>" << std::to_string(sensors_[i]->spec.type) << "<sensor/>\n";
}
for (unsigned int i = 0; i < keys_.size(); ++i) {
tree += "<key/>\n";
tree << "<key/>\n";
}
return mj_hashString(tree.c_str(), UINT64_MAX);
return mj_hashString(tree.str().c_str(), UINT64_MAX);
}
src/user/user_model.h
+2 -1
View File
@@ -19,6 +19,7 @@
#include <cstdint>
#include <functional>
#include <map>
#include <sstream>
#include <string>
#include <string_view>
#include <unordered_map>
@@ -456,7 +457,7 @@ class mjCModel : public mjCModel_, private mjSpec {
const std::vector<T*>& list);
// print the tree of a body
std::string PrintTree(const mjCBody* body, std::string indent = "");
void PrintTree(std::stringstream& tree, const mjCBody* body, int depth = 0);
// generate a signature for the model
uint64_t Signature();
src/xml/xml_native_reader.cc
+77 -29
View File
@@ -1401,7 +1401,9 @@ void mjXReader::OneFlex(XMLElement* elem, mjsFlex* flex) {
// read attributes
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(flex->element, name.c_str());
if (mjs_setName(flex->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
if (ReadAttrTxt(elem, "material", material)) {
mjs_setString(flex->material, material.c_str());
@@ -1495,7 +1497,9 @@ void mjXReader::OneMesh(XMLElement* elem, mjsMesh* mesh, const mjVFS* vfs) {
// read attributes
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(mesh->element, name.c_str());
if (mjs_setName(mesh->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
if (ReadAttrTxt(elem, "content_type", content_type)) {
*mesh->content_type = content_type;
@@ -1568,7 +1572,7 @@ void mjXReader::OneMesh(XMLElement* elem, mjsMesh* mesh, const mjVFS* vfs) {
throw mjXError(elem, "builtin mesh cannot be used with user vertex data");
}
if (mjs_makeMesh(mesh, (mjtMeshBuiltin)n, params.data(), nparams)) {
throw mjXError(elem, mjs_getError(spec));
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
@@ -1590,7 +1594,9 @@ void mjXReader::OneSkin(XMLElement* elem, mjsSkin* skin, const mjVFS* vfs) {
// read attributes
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(skin->element, name.c_str());
if (mjs_setName(skin->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
auto file = ReadAttrFile(elem, "file", vfs, AssetDir());
if (file.has_value()) {
@@ -1680,7 +1686,9 @@ void mjXReader::OneMaterial(XMLElement* elem, mjsMaterial* material) {
// read attributes
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(material->element, name.c_str());
if (mjs_setName(material->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
bool tex_attributes_found = false;
@@ -1729,7 +1737,9 @@ void mjXReader::OneJoint(XMLElement* elem, mjsJoint* joint) {
// read attributes
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(joint->element, name.c_str());
if (mjs_setName(joint->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
if (MapValue(elem, "type", &n, joint_map, joint_sz)) {
joint->type = (mjtJoint)n;
@@ -1777,7 +1787,9 @@ void mjXReader::OneGeom(XMLElement* elem, mjsGeom* geom) {
// read attributes
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(geom->element, name.c_str());
if (mjs_setName(geom->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
if (MapValue(elem, "type", &n, geom_map, mjNGEOMTYPES)) {
geom->type = (mjtGeom)n;
@@ -1849,7 +1861,9 @@ void mjXReader::OneSite(XMLElement* elem, mjsSite* site) {
// read attributes
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(site->element, name.c_str());
if (mjs_setName(site->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
if (MapValue(elem, "type", &n, geom_map, mjNGEOMTYPES)) {
site->type = (mjtGeom)n;
@@ -1882,7 +1896,9 @@ void mjXReader::OneCamera(XMLElement* elem, mjsCamera* camera) {
// read attributes
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(camera->element, name.c_str());
if (mjs_setName(camera->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
if (ReadAttrTxt(elem, "target", targetbody)) {
mjs_setString(camera->targetbody, targetbody.c_str());
@@ -1937,7 +1953,9 @@ void mjXReader::OneLight(XMLElement* elem, mjsLight* light) {
// read attributes
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(light->element, name.c_str());
if (mjs_setName(light->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
if (ReadAttrTxt(elem, "texture", texture)) {
mjs_setString(light->texture, texture.c_str());
@@ -1998,7 +2016,9 @@ void mjXReader::OnePair(XMLElement* elem, mjsPair* pair) {
// read other parameters
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(pair->element, name.c_str());
if (mjs_setName(pair->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
ReadAttrInt(elem, "condim", &pair->condim);
ReadAttr(elem, "solref", mjNREF, pair->solref, text, false, false);
@@ -2026,7 +2046,9 @@ void mjXReader::OneEquality(XMLElement* elem, mjsEquality* equality) {
// regular only
if (!readingdefaults) {
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(equality->element, name.c_str());
if (mjs_setName(equality->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
switch (equality->type) {
@@ -2162,7 +2184,9 @@ void mjXReader::OneTendon(XMLElement* elem, mjsTendon* tendon) {
// read attributes
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(tendon->element, name.c_str());
if (mjs_setName(tendon->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
ReadAttrInt(elem, "group", &tendon->group);
if (ReadAttrTxt(elem, "material", material)) {
@@ -2205,7 +2229,9 @@ void mjXReader::OneActuator(XMLElement* elem, mjsActuator* actuator) {
// common attributes
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(actuator->element, name.c_str());
if (mjs_setName(actuator->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
ReadAttrInt(elem, "group", &actuator->group);
MapValue(elem, "ctrllimited", &actuator->ctrllimited, TFAuto_map, 3);
@@ -2956,7 +2982,9 @@ void mjXReader::Custom(XMLElement* section) {
// read attributes
ReadAttrTxt(elem, "name", elname, true);
mjs_setName(numeric->element, elname.c_str());
if (mjs_setName(numeric->element, elname.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
if (ReadAttrInt(elem, "size", &numeric->size)) {
int sz = numeric->size < 500 ? numeric->size : 500;
for (int i=0; i < sz; i++) {
@@ -2987,7 +3015,9 @@ void mjXReader::Custom(XMLElement* section) {
// read attributes
ReadAttrTxt(elem, "name", elname, true);
mjs_setName(text->element, elname.c_str());
if (mjs_setName(text->element, elname.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
ReadAttrTxt(elem, "data", str, true);
if (str.empty()) {
throw mjXError(elem, "text field cannot be empty");
@@ -3007,7 +3037,9 @@ void mjXReader::Custom(XMLElement* section) {
// read attributes
ReadAttrTxt(elem, "name", elname, true);
mjs_setName(tuple->element, elname.c_str());
if (mjs_setName(tuple->element, elname.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
// read objects and add
XMLElement* obj = FirstChildElement(elem);
@@ -3223,7 +3255,9 @@ void mjXReader::Asset(XMLElement* section, const mjVFS* vfs) {
texture->colorspace = (mjtColorSpace)n;
}
if (ReadAttrTxt(elem, "name", texname)) {
mjs_setName(texture->element, texname.c_str());
if (mjs_setName(texture->element, texname.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
if (ReadAttrTxt(elem, "content_type", content_type)) {
mjs_setString(texture->content_type, content_type.c_str());
@@ -3315,7 +3349,9 @@ void mjXReader::Asset(XMLElement* section, const mjVFS* vfs) {
// read attributes
string name, content_type;
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(hfield->element, name.c_str());
if (mjs_setName(hfield->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
if (ReadAttrTxt(elem, "content_type", content_type)) {
mjs_setString(hfield->content_type, content_type.c_str());
@@ -3482,7 +3518,9 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame,
// read attributes
string name;
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(joint->element, name.c_str());
if (mjs_setName(joint->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
ReadAttrInt(elem, "group", &joint->group);
MapValue(elem, "align", &joint->align, TFAuto_map, 3);
@@ -3554,7 +3592,9 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame,
// read attributes
string name, childclass;
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(pframe->element, name.c_str());
if (mjs_setName(pframe->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
if (ReadAttrTxt(elem, "childclass", childclass)) {
mjs_setString(pframe->childclass, childclass.c_str());
@@ -3626,13 +3666,13 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame,
// attach to parent
if (!mjs_attach(body->element, pframe->element, /*prefix=*/"", suffix.c_str())) {
throw mjXError(elem, mjs_getError(spec));
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
// delete subtree
if (mjs_delete(spec, subtree->element)) {
throw mjXError(elem, mjs_getError(spec));
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
@@ -3655,7 +3695,9 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame,
// read attributes
string name, childclass;
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(child->element, name.c_str());
if (mjs_setName(child->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
if (ReadAttrTxt(elem, "childclass", childclass)) {
mjs_setString(child->childclass, childclass.c_str());
@@ -3708,12 +3750,12 @@ void mjXReader::Body(XMLElement* section, mjsBody* body, mjsFrame* frame,
child = child_body->element;
}
if (!mjs_attach(pframe->element, child, prefix.c_str(), "")) {
throw mjXError(elem, mjs_getError(spec));
throw mjXError(elem, "%s", mjs_getError(spec));
}
} else {
// only set frame to existing body
if (mjs_setFrame(child_body->element, pframe)) {
throw mjXError(elem, mjs_getError(spec));
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
}
@@ -3764,7 +3806,9 @@ void mjXReader::Contact(XMLElement* section) {
// read name and body names
if (ReadAttrTxt(elem, "name", exname)) {
mjs_setName(exclude->element, exname.c_str());
if (mjs_setName(exclude->element, exname.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
ReadAttrTxt(elem, "body1", exbody1, true);
mjs_setString(exclude->bodyname1, exbody1.c_str());
@@ -3946,7 +3990,9 @@ void mjXReader::Sensor(XMLElement* section) {
// read name, noise, userdata
if (ReadAttrTxt(elem, "name", name)) {
mjs_setName(sensor->element, name.c_str());
if (mjs_setName(sensor->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
}
ReadAttr(elem, "cutoff", 1, &sensor->cutoff, text);
ReadAttr(elem, "noise", 1, &sensor->noise, text);
@@ -4377,7 +4423,9 @@ void mjXReader::Keyframe(XMLElement* section) {
// read name, time
ReadAttrTxt(elem, "name", name);
mjs_setName(key->element, name.c_str());
if (mjs_setName(key->element, name.c_str())) {
throw mjXError(elem, "%s", mjs_getError(spec));
}
ReadAttr(elem, "time", 1, &key->time, text);
// read qpos
test/CMakeLists.txt
+1
View File
@@ -42,6 +42,7 @@ macro(mujoco_test name)
)
add_executable(${name} ${name}.cc)
target_link_options(${name} PRIVATE ${EXTRA_LINK_OPTIONS})
target_link_libraries(${name} mujoco fixture gmock)
if(_ARGS_MAIN_TARGET)
target_link_libraries(${name} ${_ARGS_MAIN_TARGET})
test/xml/xml_native_reader_test.cc
+1 -1
View File
@@ -1529,7 +1529,7 @@ TEST_F(XMLReaderTest, ParseReplicateRepeatedName) {
mjSpec* spec = mj_parseXMLString(xml, 0, error.data(), error.size());
EXPECT_THAT(spec, IsNull()) << error.data();
EXPECT_THAT(error.data(), HasSubstr("repeated name 'b' in actuator"));
EXPECT_THAT(error.data(), HasSubstr("Element 'replicate'"));
EXPECT_THAT(error.data(), HasSubstr("Element 'position'"));
}
TEST_F(XMLReaderTest, RepeatedPrefix) {
test/xml/xml_native_writer_test.cc
+2
View File
@@ -1417,6 +1417,8 @@ TEST_F(XMLWriterTest, WriteReadCompare) {
auto abs_path = p.path();
mjSpec* stemp = mj_parseXMLString(SaveAndReadXml(s).c_str(), 0,
error.data(), error.size());
ASSERT_THAT(stemp, NotNull())
<< "Failed to load " << xml.c_str() << ": " << error.data();
mjs_setString(stemp->modelfiledir,
abs_path.remove_filename().string().c_str());
mjModel* mtemp = mj_compile(stemp, nullptr);