Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The CTM export routine for ctmconv can access out of bounds #21

Open
musicinmybrain opened this issue Apr 9, 2024 · 0 comments
Open

Comments

@musicinmybrain
Copy link

The easiest way to observe this is to compile with libstdc++ assertions enabled. In Makefile.linux, add -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS to the CPPFLAGS. (This is part of the distribution default compiler flags in Fedora).

Now,

$ make -f Makefile.linux
$ curl -L -O https://github.com/gazebosim/gazebo-classic/raw/gazebo10_10.1.0/media/models/sitting.dae
$ LD_LIBRARY_PATH="$PWD/lib" ./tools/ctmconv sitting.dae sitting.ctm
Loading sitting.dae... 47.15 ms
Saving sitting.ctm... /usr/include/c++/13/bits/stl_vector.h:1125: std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _Tp = int; _Alloc = std::allocator<int>; reference = int&; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
Aborted (core dumped)

If we remove -s so the executable is not stripped of debug symbols:

$(CPP) -s -o $@ -L$(OPENCTMDIR) -L$(TINYXMLDIR) $(CTMCONVOBJS) -Wl,-rpath,. -lopenctm -ltinyxml

…and furthermore add -g -Og to the CPPFLAGS, then recompile, then we can try again with gdb and get a really nice backtrace:

$ LD_LIBRARY_PATH="$PWD/lib" gdb --args ./tools/ctmconv sitting.dae sitting.ctm
[…]
(gdb) bt
#0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
#1  0x00007ffff7aae8a3 in __pthread_kill_internal (signo=6, threadid=<optimized out>) at pthread_kill.c:78
#2  0x00007ffff7a5c8ee in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#3  0x00007ffff7a448ff in __GI_abort () at abort.c:79
#4  0x00007ffff7cd95b0 in std::__glibcxx_assert_fail (file=file@entry=0x4250e8 "/usr/include/c++/13/bits/stl_vector.h", line=line@entry=1125, 
    function=function@entry=0x425110 "std::vector<_Tp, _Alloc>::reference std::vector<_Tp, _Alloc>::operator[](size_type) [with _Tp = int; _Alloc = std::allocator<int>; reference = int&; size_type = long unsigned int]", condition=condition@entry=0x425010 "__n < this->size()") at ../../../../../libstdc++-v3/src/c++11/assert_fail.cc:41
#5  0x00000000004062d9 in std::vector<int, std::allocator<int> >::operator[] (this=this@entry=0x7fffffffd298, __n=__n@entry=0) at /usr/include/c++/13/bits/stl_vector.h:1125
#6  0x0000000000409890 in Export_CTM (aFileName=aFileName@entry=0x7fffffffd340 "sitting.ctm", aMesh=aMesh@entry=0x7fffffffd250, aOptions=...) at ctm.cpp:127
#7  0x000000000040921a in ExportMesh (aFileName=0x7fffffffd340 "sitting.ctm", aMesh=aMesh@entry=0x7fffffffd250, aOptions=...) at meshio.cpp:82
#8  0x0000000000405c64 in main (argc=3, argv=0x7fffffffd538) at /usr/include/c++/13/bits/basic_string.h:222
(gdb) frame 6
#6  0x0000000000409890 in Export_CTM (aFileName=aFileName@entry=0x7fffffffd340 "sitting.ctm", aMesh=aMesh@entry=0x7fffffffd250, aOptions=...) at ctm.cpp:127
127                      (const CTMuint*) &aMesh->mIndices[0], aMesh->mIndices.size() / 3,
 print *aMesh
$2 = {mOriginalNormals = true, mComment = "", mTexFileName = "", mIndices = std::vector of length 0, capacity 0, mVertices = std::vector of length 0, capacity 0, 
  mNormals = std::vector of length 0, capacity 0, mColors = std::vector of length 0, capacity 0, mAttributes = std::vector of length 0, capacity 0, mTexCoords = std::vector of length 0, capacity 0, 
  attributesName = 0x0}

Something has apparently gone wrong in importing the mesh, which might be its own bug, and the aMesh structure has all empty vectors. The problem then is that the export code unconditionally takes the addresses of the first elements in

OpenCTM/tools/ctm.cpp

Lines 126 to 128 in 91b3b71

ctm.DefineMesh((CTMfloat *) &aMesh->mVertices[0].x, aMesh->mVertices.size(),
(const CTMuint*) &aMesh->mIndices[0], aMesh->mIndices.size() / 3,
normals);

which is undefined on an empty/zero-element vector even if the pointer is never dereferenced. For &aMesh->mIndices[0], we could use amesh->mIndices.data(), which should be at least be OK to call on an empty vector, but we can’t apply an offset to the result to replace &aMesh->mVertices[0].x.

One workaround could be to define “dummy” CTMFloat and CTMuint values locally, and point to them when the vertex and index vectors are empty, respectively.

@musicinmybrain musicinmybrain changed the title The CTM export routine for ctmconv can read out of bounds The CTM export routine for ctmconv can access out of bounds Apr 9, 2024
musicinmybrain added a commit to musicinmybrain/OpenCTM-1 that referenced this issue Apr 9, 2024
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

No branches or pull requests

1 participant