Fix segfault in global tractography. #998

dchristiaens · 2017-05-24T14:08:47Z

As reported in #992, the recent merge of #785 accidentally introduced a segfault in tckglobal. This pull requests fixes this.

The error was caused by two issues:

The new row method for image access interfered with an out-of-bounds check on the third dimension (DWI volumes) in the data energy computation. This was easily fixed by limiting the bounds check to the spatial dimensions.
The memory alignment issues required a dedicated aligned allocator in std::deque and std::stack members of the object pool used to store track segments. This was fixed in 860c474 by setting the appropriate template arguments.

Today's commits clean up the code without affecting performance.

ansk · 2017-05-24T16:57:42Z

Dear experts,

my mrtrix build after this merge exits with following error:


ERROR: ( 76/495) [CC] tmp/src/dwi/tractography/GT/particlegrid.o

g++ -c -std=c++11 -pthread -fPIC -DMRTRIX_WORD64 -Wall -O3 -DNDEBUG -Isrc -I./core -Icmd -isystem /usr/include/eigen3 -DEIGEN_DONT_PARALLELIZE src/dwi/tractography/GT/particlegrid.cpp -o tmp/src/dwi/tractography/GT/particlegrid.o

failed with output

In file included from /usr/include/c++/4.9/deque:66:0,
                 from src/dwi/tractography/GT/particlepool.h:18,
                 from src/dwi/tractography/GT/particlegrid.h:26,
                 from src/dwi/tractography/GT/particlegrid.cpp:15:
/usr/include/c++/4.9/bits/deque.tcc: In instantiation of void std::deque<_Tp, _Alloc>::emplace_back(_Args&& ...) [with _Args = {const Eigen::Matrix<float, 3, 1, 0, 3, 1>&, const Eigen::Matrix<float, 3, 1, 0, 3, 1>&}; _Tp = MR::DWI::Tractography::GT::Particle; _Alloc = Eigen::aligned_allocator<MR::DWI::Tractography::GT::Particle>]:
src/dwi/tractography/GT/particlepool.h:51:41:   required from here
/usr/include/c++/4.9/bits/deque.tcc:137:6: error: no matching function for call to std::_Deque_base<MR::DWI::Tractography::GT::Particle, Eigen::aligned_allocator<MR::DWI::Tractography::GT::Particle> >::_Deque_impl::construct(MR::DWI::Tractography::GT::Particle*&, const Eigen::Matrix<float, 3, 1>&, const Eigen::Matrix<float, 3, 1>&)
      this->_M_impl.construct(this->_M_impl._M_finish._M_cur,
      ^
/usr/include/c++/4.9/bits/deque.tcc:137:6: note: candidate is:
In file included from /usr/include/eigen3/Eigen/Core:256:0,
                 from /usr/include/eigen3/Eigen/Geometry:4,
                 from ./core/types.h:63,
                 from ./core/mrtrix.h:36,
                 from ./core/cmdline_option.h:27,
                 from ./core/app.h:27,
                 from ./core/header.h:21,
                 from src/dwi/tractography/GT/particlegrid.h:20,
                 from src/dwi/tractography/GT/particlegrid.cpp:15:
/usr/include/eigen3/Eigen/src/Core/util/Memory.h:729:10: note: void Eigen::aligned_allocator<T>::construct(Eigen::aligned_allocator<T>::pointer, const T&) [with T = MR::DWI::Tractography::GT::Particle; Eigen::aligned_allocator<T>::pointer = MR::DWI::Tractography::GT::Particle*]
     void construct( pointer p, const T& value )
          ^
/usr/include/eigen3/Eigen/src/Core/util/Memory.h:729:10: note:   candidate expects 2 arguments, 3 provided

Content of my configure.log:


REPORT: 
MRtrix build type requested:

REPORT: release

REPORT: 

REPORT: Detecting OS: linux

REPORT: Looking for compiler [clang++]:
EXEC <<
CMD: clang++ --version
error invoking command "clang++": No such file or directory
>>


REPORT: not found

REPORT: Looking for compiler [g++]:
EXEC <<
CMD: g++ --version
EXIT: 0
STDOUT:
g++ (Debian 4.9.2-10) 4.9.2
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>>


REPORT: g++ (Debian 4.9.2-10) 4.9.2

REPORT: Checking for C++11 compliance:

COMPILE /tmp/tmpnWeR_T.cpp:
---

#include <cstddef>
struct Base {
    Base (int);
};
struct Derived : Base {
    using Base::Base;
};

int main() {
  Derived D (int); // check for contructor inheritance
  return (0);
}

---
EXEC <<
CMD: g++ -c -std=c++11 -pthread -fPIC /tmp/tmpnWeR_T.cpp -o /tmp/tmpnWeR_T.o
EXIT: 0
>>

EXEC <<
CMD: g++ /tmp/tmpnWeR_T.o -pthread -o a.out
EXIT: 0
>>

EXEC <<
CMD: ./a.out
EXIT: 0
>>


REPORT: ok

REPORT: Checking for ::max_align_t:

COMPILE /tmp/tmp2F8vVL.cpp:
---

#include <cstddef>
using ::max_align_t;
int main() {
  return 0;
}

---
EXEC <<
CMD: g++ -c -std=c++11 -pthread -fPIC /tmp/tmp2F8vVL.cpp -o /tmp/tmp2F8vVL.o
EXIT: 0
>>

EXEC <<
CMD: g++ /tmp/tmp2F8vVL.o -pthread -o a.out
EXIT: 0
>>

EXEC <<
CMD: ./a.out
EXIT: 0
>>


REPORT: ok

REPORT: Checking for std::max_align_t:

COMPILE /tmp/tmpB0xoqJ.cpp:
---

#include <cstddef>
using std::max_align_t;
int main() {
  return 0;
}

---
EXEC <<
CMD: g++ -c -std=c++11 -pthread -fPIC /tmp/tmpB0xoqJ.cpp -o /tmp/tmpB0xoqJ.o
EXIT: 0
>>

EXEC <<
CMD: g++ /tmp/tmpB0xoqJ.o -pthread -o a.out
EXIT: 0
>>

EXEC <<
CMD: ./a.out
EXIT: 0
>>


REPORT: ok

REPORT: Detecting pointer size:

COMPILE /tmp/tmpyWq_38.cpp:
---

#include <iostream>
int main() {
  std::cout << sizeof(void*);
  return (0);
}

---
EXEC <<
CMD: g++ -c -std=c++11 -pthread -fPIC /tmp/tmpyWq_38.cpp -o /tmp/tmpyWq_38.o
EXIT: 0
>>

EXEC <<
CMD: g++ /tmp/tmpyWq_38.o -pthread -o a.out
EXIT: 0
>>

EXEC <<
CMD: ./a.out
EXIT: 0
STDOUT:
8
>>


REPORT: 64 bit

REPORT: Detecting byte order:

REPORT: little-endian

REPORT: Checking for variable-length array support:

COMPILE /tmp/tmp8J0z5o.cpp:
---


int main(int argc, char* argv[]) {
  int x[argc];
  return 0;
}

---
EXEC <<
CMD: g++ -c -std=c++11 -pthread -fPIC -DMRTRIX_WORD64 /tmp/tmp8J0z5o.cpp -o /tmp/tmp8J0z5o.o
EXIT: 0
>>

EXEC <<
CMD: g++ /tmp/tmp8J0z5o.o -pthread -o a.out
EXIT: 0
>>

EXEC <<
CMD: ./a.out
EXIT: 0
>>


REPORT: yes

REPORT: Checking for non-POD variable-length array support:

COMPILE /tmp/tmpA7frjv.cpp:
---

#include <string>

class X {
  int x;
  double y;
  std::string s;
};

int main(int argc, char* argv[]) {
  X x[argc];
  return 0;
}

---
EXEC <<
CMD: g++ -c -std=c++11 -pthread -fPIC -DMRTRIX_WORD64 /tmp/tmpA7frjv.cpp -o /tmp/tmpA7frjv.o
EXIT: 0
>>

EXEC <<
CMD: g++ /tmp/tmpA7frjv.o -pthread -o a.out
EXIT: 0
>>

EXEC <<
CMD: ./a.out
EXIT: 0
>>


REPORT: yes

REPORT: Checking for zlib compression library:

COMPILE /tmp/tmpy_yVO9.cpp:
---

#include <iostream>
#include <zlib.h>

int main() {
  std::cout << zlibVersion();
  return (0);
}

---
EXEC <<
CMD: g++ -c -std=c++11 -pthread -fPIC -DMRTRIX_WORD64 /tmp/tmpy_yVO9.cpp -o /tmp/tmpy_yVO9.o
EXIT: 0
>>

EXEC <<
CMD: g++ /tmp/tmpy_yVO9.o -pthread -lz -o a.out
EXIT: 0
>>

EXEC <<
CMD: ./a.out
EXIT: 0
STDOUT:
1.2.8
>>


REPORT: 1.2.8

REPORT: Checking for Eigen 3 library:
EXEC <<
CMD: pkg-config --cflags eigen3
EXIT: 0
STDOUT:
-I/usr/include/eigen3
>>


COMPILE /tmp/tmpzQj18f.cpp:
---

#include <cstddef>
#include <Eigen/Core>
#include <iostream>

int main (int argc, char* argv[]) {
  std::cout << EIGEN_WORLD_VERSION << "." << EIGEN_MAJOR_VERSION << "." << EIGEN_MINOR_VERSION << "\n";
  return 0;
}

---
EXEC <<
CMD: g++ -c -std=c++11 -pthread -fPIC -DMRTRIX_WORD64 -isystem /usr/include/eigen3 /tmp/tmpzQj18f.cpp -o /tmp/tmpzQj18f.o
EXIT: 0
>>

EXEC <<
CMD: g++ /tmp/tmpzQj18f.o -pthread -lz -o a.out
EXIT: 0
>>

EXEC <<
CMD: ./a.out
EXIT: 0
STDOUT:
3.2.2
>>


REPORT: 3.2.2

REPORT: Checking JSON for Modern C++ requirements:

COMPILE /tmp/tmpsg7vFf.cpp:
---

#include "file/json.h"

int main (int argc, char* argv[])
{
  nlohmann::json json;
  json["key"] = "value";
}


---
EXEC <<
CMD: g++ -c -std=c++11 -pthread -fPIC -DMRTRIX_WORD64 -I/hydradb/hydra_io/vypocty/skoch/bin/mrtrix3/core /tmp/tmpsg7vFf.cpp -o /tmp/tmpsg7vFf.o
EXIT: 0
>>

EXEC <<
CMD: g++ /tmp/tmpsg7vFf.o -pthread -lz -o a.out
EXIT: 0
>>

EXEC <<
CMD: ./a.out
EXIT: 0
>>


REPORT: OK

REPORT: Checking shared library generation:
EXEC <<
CMD: g++ -c -std=c++11 -pthread -fPIC -DMRTRIX_WORD64 /tmp/tmpWujo3s.cpp -o /tmp/tmpWujo3s.o
EXIT: 0
>>

EXEC <<
CMD: g++ /tmp/tmpWujo3s.o -pthread -shared -pthread -lz -o libtest.so
EXIT: 0
>>


REPORT: yes

REPORT: Checking for Qt moc:
EXEC <<
CMD: moc -v
EXIT: 1
STDERR:
Qt Meta Object Compiler version 63 (Qt 4.8.6)
>>


REPORT: moc (version 4.8.6)

REPORT: Checking for Qt qmake:
EXEC <<
CMD: qmake -v
EXIT: 0
STDOUT:
QMake version 2.01a
Using Qt version 4.8.6 in /usr/lib/x86_64-linux-gnu
>>


REPORT: qmake (version 4.8.6)

REPORT: Checking for Qt rcc:
EXEC <<
CMD: rcc -v
EXIT: 1
STDERR:
Qt Resource Compiler version 4.8.6
>>


REPORT: rcc (version 4.8.6)

REPORT: Checking for Qt:

source file "qt.h":
---
#include <QObject>

class Foo: public QObject {
  Q_OBJECT;
  public:
    Foo();
    ~Foo();
  public slots:
    void setValue(int value);
  signals:
    void valueChanged (int newValue);
  private:
    int value_;
};
---

source file "qt.cpp":
---
#include <iostream>
#include "qt.h"

Foo::Foo() : value_ (42) { connect (this, SIGNAL(valueChanged(int)), this, SLOT(setValue(int))); }

Foo::~Foo() { std::cout << qVersion() << "\n"; }

void Foo::setValue (int value) { value_ = value; }

int main() { Foo f; }
---

project file "qt.pro":
---
CONFIG += c++11
QT += core gui opengl svg
HEADERS += qt.h
SOURCES += qt.cpp
---
EXEC <<
CMD: qmake
EXIT: 0
>>

EXEC <<
CMD: moc qt.h -o qt_moc.cpp
EXIT: 0
>>

EXEC <<
CMD: g++ -c -std=c++11 -pthread -fPIC -DMRTRIX_WORD64 -m64 -pipe -Wall -W -D_REENTRANT -DQT_WEBKIT -DQT_NO_DEBUG -DQT_SVG_LIB -DQT_OPENGL_LIB -DQT_GUI_LIB -DQT_CORE_LIB -DQT_SHARED -isystem /usr/share/qt4/mkspecs/linux-g++-64 -isystem /usr/include/qt4/QtCore -isystem /usr/include/qt4/QtGui -isystem /usr/include/qt4/QtOpenGL -isystem /usr/include/qt4/QtSvg -isystem /usr/include/qt4 -isystem /usr/X11R6/include qt.cpp -o qt.o
EXIT: 0
>>

EXEC <<
CMD: g++ -c -std=c++11 -pthread -fPIC -DMRTRIX_WORD64 -m64 -pipe -Wall -W -D_REENTRANT -DQT_WEBKIT -DQT_NO_DEBUG -DQT_SVG_LIB -DQT_OPENGL_LIB -DQT_GUI_LIB -DQT_CORE_LIB -DQT_SHARED -isystem /usr/share/qt4/mkspecs/linux-g++-64 -isystem /usr/include/qt4/QtCore -isystem /usr/include/qt4/QtGui -isystem /usr/include/qt4/QtOpenGL -isystem /usr/include/qt4/QtSvg -isystem /usr/include/qt4 -isystem /usr/X11R6/include qt_moc.cpp -o qt_moc.o
EXIT: 0
>>

EXEC <<
CMD: g++ -pthread -lz qt_moc.o qt.o -o qt -m64 -Wl,-O1 -L/usr/lib/x86_64-linux-gnu -L/usr/X11R6/lib64 -lQtSvg -lQtOpenGL -lQtGui -lQtCore -lGL -lpthread
EXIT: 0
>>

EXEC <<
CMD: /tmp/tmpmYAyxV/qt
EXIT: 0
STDOUT:
4.8.6
>>


REPORT: 4.8.6

dchristiaens · 2017-05-24T17:10:49Z

Strange. Do you also get the error when building against Eigen 3.3?

dchristiaens · 2017-05-24T18:14:10Z

@ansk, does #999 fix it for you? It's cleaner anyway...

jdtournier · 2017-05-25T12:42:49Z

OK, had a quick look into this. It seems the issue is that Eigen's allocator in the older version is not fully C++11 compliant. To expand:

an allocator object (Eigen::aligned_allocator in this instance) needs to provide allocate() and construct() calls (although the latter is deprecated in C++17). The signature of the construct() call itself is however version-dependent (see this page). The error you're seing is consistent with the older construct() method being used, yet we're compiling in C++11:

/usr/include/eigen3/Eigen/src/Core/util/Memory.h:729:10: note: void Eigen::aligned_allocator<T>::construct(Eigen::aligned_allocator<T>::pointer, const T&) [with T = MR::DWI::Tractography::GT::Particle; Eigen::aligned_allocator<T>::pointer = MR::DWI::Tractography::GT::Particle*]
     void construct( pointer p, const T& value )
          ^
/usr/include/eigen3/Eigen/src/Core/util/Memory.h:729:10: note:   candidate expects 2 arguments, 3 provided

In C++11, this call would take a variadic argument passed directly through from the C++11 emplace_back() call, and all would be well (it's supposed to invoke the class's matching constructor, passing it the additional arguments). The older construct() call however expects to invoke the copy constructor. So it seems to me that the simplest fix would be to change this line:

 51               pool.emplace_back(pos, dir);

to:

 51               pool.emplace_back (Particle (pos, dir));

which should work for both versions... (?) I don't think there'll be any performance penalty thanks to copy elision.

dchristiaens added 4 commits May 23, 2017 14:43

tckglobal: Fix out-of-bounds issue introduced with the new row method.

dde81b5

tckglobal: Fix memory alignment in deque and stack.

860c474

tckglobal: cleanup particlegrid accessors.

5faf06d

tckglobal: small docs update

47b9fec

dchristiaens added bug critical labels May 24, 2017

dchristiaens added this to the MRtrix3 3.0 release milestone May 24, 2017

This was referenced May 24, 2017

Global tractography threading race condition. #477

Closed

tckglobal Segmentation fault: Invalid memory access in MH Sampler #992

Closed

dchristiaens merged commit b19f24a into master May 24, 2017

jdtournier deleted the tckglobal_fixes branch May 24, 2017 16:09

dchristiaens restored the tckglobal_fixes branch May 24, 2017 17:57

dchristiaens mentioned this pull request May 24, 2017

Move memory aligned deque to core/types.h #999

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix segfault in global tractography. #998

Fix segfault in global tractography. #998

Uh oh!

dchristiaens commented May 24, 2017

Uh oh!

ansk commented May 24, 2017 •

edited

Loading

Uh oh!

dchristiaens commented May 24, 2017

Uh oh!

dchristiaens commented May 24, 2017

Uh oh!

jdtournier commented May 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix segfault in global tractography. #998

Fix segfault in global tractography. #998

Uh oh!

Conversation

dchristiaens commented May 24, 2017

Uh oh!

ansk commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dchristiaens commented May 24, 2017

Uh oh!

dchristiaens commented May 24, 2017

Uh oh!

jdtournier commented May 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ansk commented May 24, 2017 •

edited

Loading