Idyntree: Use of not initialized memory when using IPOPT < 3.12.11 with MUMPS >= 5.1.0

[traversaro]

~
85: Test command: /usr/bin/valgrind "--leak-check=full" "--error-exitcode=1" "/home/straversaro/src/robotology-superbuild/build/robotology/iDynTree/bin/OptimizerUnitTest"
85: Test timeout computed to be: 1500
85: ==18723== Memcheck, a memory error detector
85: ==18723== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
85: ==18723== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
85: ==18723== Command: /home/straversaro/src/robotology-superbuild/build/robotology/iDynTree/bin/OptimizerUnitTest
85: ==18723==
85: ==18723== Conditional jump or move depends on uninitialised value(s)
85: ==18723== at 0x76C4FD7: dmumps_ (in /usr/lib/x86_64-linux-gnu/libdmumps_seq-5.1.2.so)
85: ==18723== by 0x76CA000: dmumps_f77_ (in /usr/lib/x86_64-linux-gnu/libdmumps_seq-5.1.2.so)
85: ==18723== by 0x76C2EE1: dmumps_c (in /usr/lib/x86_64-linux-gnu/libdmumps_seq-5.1.2.so)
85: ==18723== by 0x6487C86: Ipopt::MumpsSolverInterface::MumpsSolverInterface() (in /usr/lib/libipopt.so.1.9.9)
85: ==18723== by 0x63A107A: Ipopt::AlgorithmBuilder::BuildBasicAlgorithm(Ipopt::Journalist const&, Ipopt::OptionsList const&, std::__cxx11::basic_string, std::allocator > const&) (in /usr/lib/libipopt.so.1.9.9)
85: ==18723== by 0x635A175: Ipopt::IpoptApplication::OptimizeNLP(Ipopt::SmartPtr const&, Ipopt::SmartPtr&) (in /usr/lib/libipopt.so.1.9.9)
85: ==18723== by 0x63543B8: Ipopt::IpoptApplication::OptimizeNLP(Ipopt::SmartPtr const&) (in /usr/lib/libipopt.so.1.9.9)
85: ==18723== by 0x6356679: Ipopt::IpoptApplication::OptimizeTNLP(Ipopt::SmartPtr const&) (in /usr/lib/libipopt.so.1.9.9)
85: ==18723== by 0x50A5326: iDynTree::optimization::IpoptInterface::solve() (in /home/straversaro/src/robotology-superbuild/build/robotology/iDynTree/lib/libidyntree-optimalcontrol.so)
85: ==18723== by 0x10B6FB: main (in /home/straversaro/src/robotology-superbuild/build/robotology/iDynTree/bin/OptimizerUnitTest)
85: ==18723==
85:
~

Probably this is due to something inside Mumps. It probably make sense to just add it to the Valgrind suppressions. Interestingly, I cannot find the same problem reported anywhere else.

[traversaro]

Indeed, the mumps used in Ubuntu Bionic 18.04 is version 5.1.2 , while the one used in Ubuntu Xenial 16.04 was 4.10.0, so it is possible that this problem was introduce in the meanwhile.

I checked some other projects that use Ipopt to see if they found something similar, but the only one that runs test under Valgrind as part of their test suite is Drake, that however is still using xenial 16.04 for all their Jenkins tests (see https://github.com/RobotLocomotion/drake/issues/7882), so it is possible that we are the firsts encountering this in the wild.

[traversaro]

I need to investigate more about the error and properly open a Debian bug if the problem is consistent, but perhaps this could be of interest (at least as FYI) for the Debian packager of mumps, @RizzerOnGitHub .

[traversaro]

Files changed between mumps 4.10.0 and mumps 5.1.2 : https://salsa.debian.org/science-team/mumps/commit/cb40d420da8a45fccf422a16502d8ac75d6279d2 .

[drew-parsons]

Thanks for the notice. It may be a tricky bug to track down in the mumps package. Does it seem to run successfully outside valgrind?

I'm not familiar with the ipopt interface. It's possible that ipopt is expecting a mumps 4 interface (yet doesn't trigger an error against mumps 5) and that it's ipopt which needs to be upgraded to match mumps 5.

One suggestion is to rebuild both the mump package and ipopt on your own system to help ensure the binary interfaces are consistent with each other.

[traversaro]

Thanks for the notice. It may be a tricky bug to track down in the mumps package. Does it seem to run successfully outside valgrind?

Yes, beside the Valgrind warning, all the tests are successful. The actual optimization problem works fine, but if we are actually using not initialized memory this could be just luck.

One suggestion is to rebuild both the mump package and ipopt on your own system to help ensure the binary interfaces are consistent with each other.

That's my plan as soon as I will get a bit of time to spend on this. I am not really familiar with the mumps build system, so I think I will build directly the debian package from source.

Thanks a lot for the useful insight!

[drew-parsons]

That's what I meant, rebuild the debian package. dpkg-buildpackage is probably the best tool for it, it will tell you if you need to install build dependencies. Use dch to make a new changelog entry to mark it as a local rebuild (e.g. with version 5.1.2-4.1)

[drew-parsons]

Another thought is to install the libmumps-*-dbgsym packages to get mumps symbols. If valgrind can use them then they might help pin down where exactly in mumps the uninitialised value is.

[traversaro]

Another thought is to install the libmumps-*-dbgsym packages to get mumps symbols. If valgrind can use them then they might help pin down where exactly in mumps the uninitialised value is.

Apparently there is no libmumps-*-dbgsym in Ubuntu Bionic, but I see that it is available in Debian Sid. I will setup a Debian Sid system to to this test, thanks again.

[drew-parsons]

dpkg-buildpackage should generate the dbgsym, so you check rebuild and symbols at the same time

[traversaro]

I recompiled mumps and ipopt, but for some reason (fortran-related stuff?) valgrind gets the symbols only for ipopt:
~
86: Test command: /usr/bin/valgrind "--leak-check=full" "--error-exitcode=1" "/home/straversaro/src/robotology-superbuild/build/robotology/iDynTree/bin/OptimalControlIpoptUnitTest"
86: Test timeout computed to be: 1500
86: ==20537== Memcheck, a memory error detector
86: ==20537== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
86: ==20537== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
86: ==20537== Command: /home/straversaro/src/robotology-superbuild/build/robotology/iDynTree/bin/OptimalControlIpoptUnitTest
86: ==20537==
86: ==20537== Conditional jump or move depends on uninitialised value(s)
86: ==20537== at 0x76C4FD7: dmumps_ (in /usr/lib/x86_64-linux-gnu/libdmumps_seq-5.1.2.so)
86: ==20537== by 0x76CA000: dmumps_f77_ (in /usr/lib/x86_64-linux-gnu/libdmumps_seq-5.1.2.so)
86: ==20537== by 0x76C2EE1: dmumps_c (in /usr/lib/x86_64-linux-gnu/libdmumps_seq-5.1.2.so)
86: ==20537== by 0x6487C86: Ipopt::MumpsSolverInterface::MumpsSolverInterface() (IpMumpsSolverInterface.cpp:86)
86: ==20537== by 0x63A107A: Ipopt::AlgorithmBuilder::BuildBasicAlgorithm(Ipopt::Journalist const&, Ipopt::OptionsList const&, std::__cxx11::basic_string, std::allocator > const&) (IpAlgBuilder.cpp:469)
86: ==20537== by 0x635A175: Ipopt::IpoptApplication::OptimizeNLP(Ipopt::SmartPtr const&, Ipopt::SmartPtr&) (IpIpoptApplication.cpp:766)
86: ==20537== by 0x63543B8: Ipopt::IpoptApplication::OptimizeNLP(Ipopt::SmartPtr const&) (IpIpoptApplication.cpp:732)
86: ==20537== by 0x6356679: Ipopt::IpoptApplication::OptimizeTNLP(Ipopt::SmartPtr const&) (IpIpoptApplication.cpp:711)
86: ==20537== by 0x50A5326: iDynTree::optimization::IpoptInterface::solve() (in /home/straversaro/src/robotology-superbuild/build/robotology/iDynTree/lib/libidyntree-optimalcontrol.so)
86: ==20537== by 0x5070B16: iDynTree::optimalcontrol::MultipleShootingSolver::solve() (in /home/straversaro/src/robotology-superbuild/build/robotology/iDynTree/lib/libidyntree-optimalcontrol.so)
86: ==20537== by 0x10F1B8: main (in /home/straversaro/src/robotology-superbuild/build/robotology/iDynTree/bin/OptimalControlIpoptUnitTest)
~

However, I think this is sufficient to understand the root of the issue. The problem is in this line:
https://github.com/coin-or/Ipopt/blob/releases/3.11.9/Ipopt/src/Algorithm/LinearSolvers/IpMumpsSolverInterface.cpp#L86
Apparently, several new attributes were added DMUMPS_STRUC_C data structure when going from mumps 4 to mumps 5: https://salsa.debian.org/science-team/mumps/commit/cb40d420da8a45fccf422a16502d8ac75d6279d2#9c6cf360c3a08b165017b6b0d6bab1d993948d33_81_46 . Clealy ipopt is not aware of these new parameters, even because the version of mumps "vendored" inside IPOPT is still mumps 4. From a quick glance to the diff in the examples in https://salsa.debian.org/science-team/mumps/commit/cb40d420da8a45fccf422a16502d8ac75d6279d2#d8d3c66121b3fe4c8fee671d916ddf1ea363fb0c_47_45 , I guess the nnz parameter should be set, but I am not able to understand if this parameter is set to a non initialized value could create some actual problems.

[traversaro]

Ticket in IPOPT bugtracker related to supporting MUMPS 5 in the configuration process: https://projects.coin-or.org/Ipopt/ticket/301 .

[traversaro]

Anyone of the more optimization-oriented people in the lab has any insight of changes of mumps interface, and eventually if they could affect ipopt results @S-Dafarra @GiulioRomualdi ?

[S-Dafarra]

I have always used that as user. Ipopt makes the inner linear solver quite transparent for the end user. In principle the choice of linear solver should not affect the goodness of the result. We may test this by switching solvers.

I guess that parameter specifes the number of nonzeros but it should have been set by Ipopt also in previous versions of mumps.

[S-Dafarra]

As pointed in the issue you linked, version 5 is not supported. Who is installing that version? Can we downgrade to mumps 4?

[traversaro]

Ok, I think I got it. It seems an actual problem.

I guess that parameter specifes the number of nonzeros but it should have been set by Ipopt also in previous versions of mumps.

Exactly. that is the number of nonzeros values. The MUMPS user's guide is actually quite clear. However, there has been a change in MUMPS 5.1.0, the support for a 64-bit integer to specify the number of nonzero values (https://salsa.debian.org/science-team/mumps/commit/cb40d420da8a45fccf422a16502d8ac75d6279d2#00fe7828d56d7a3ee4030d6cea057cf13f50e70c_5_13) using the nnz parameter in place of the old nz one :

• New feature: selective 64-bit integers (introduced only where needed)
  to process matrices with more than 2^{31}-1 entries.
  mixed 32/64 bit integers for API: NNZ/NNZ_loc 64-bit
  (NZ/NZ_LOC kept temporarily for backward compatibility)
  both 32 or 64 bit integer versions of external orderings
  (Metis/ParMetis, SCOTCH/pt-SCOTCH, PORD), can be used
  Error -51 when a 32-bit external ordering is invoked on
  a graph larger than 2^{31}-1

Apparently, there is backward compatibility for the old nz parameter. However, how this backward compatibility works? Let's check in the MUMPS_GET_NNZ_INTERNAL subroutine ( https://salsa.debian.org/science-team/mumps/commit/cb40d420da8a45fccf422a16502d8ac75d6279d2#b3ca548083b9cd8c7dbe469c41cc60457d94bb46_0_824 ):
~
SUBROUTINE MUMPS_GET_NNZ_INTERNAL( NNZ, NZ, NNZ_i )
INTEGER , INTENT(IN) :: NZ
INTEGER(8), INTENT(IN) :: NNZ
INTEGER(8), INTENT(OUT) :: NNZ_i
IF (NNZ > 0_8) THEN
NNZ_i = NNZ
ELSE
NNZ_i = int(NZ, 8)
ENDIF
END SUBROUTINE MUMPS_GET_NNZ_INTERNAL
~
In a nutshell, if NNZ is greater than zero, its value is used, otherwise the old nz value is used. I still need to double check, but this means that the tests are running fine because the not initialized memory of nnz is equal to zero or some negative number, but it would be perfectly possible for the not initialized memory to have a positive value and probably cause a crash in mumps.

[traversaro]

Testing the following fix:
~~~patch
--- coinor-ipopt-3.11.9.orig/Ipopt/src/Algorithm/LinearSolvers/IpMumpsSolverInterface.cpp
+++ coinor-ipopt-3.11.9/Ipopt/src/Algorithm/LinearSolvers/IpMumpsSolverInterface.cpp
@@ -76,6 +76,7 @@ namespace Ipopt
#endif
mumps_->n = 0;
mumps_->nz = 0;

• mumps_->nnz = 0;
  mumps_->a = NULL;
  mumps_->jcn = NULL;
  mumps_->irn = NULL;
  @@ -201,6 +202,7 @@ namespace Ipopt
  if (!warm_start_same_structure_) {
  mumps_->n = 0;
  mumps_->nz = 0;
• mumps_->nnz = 0;
  }
  else {
  ASSERT_EXCEPTION(mumps_->n>0 && mumps_->nz>0, INVALID_WARMSTART,
  @@ -309,6 +311,7 @@ namespace Ipopt
  if (!warm_start_same_structure_) {
  mumps_->n = dim;
  mumps_->nz = nonzeros;
• mumps_->nnz = nonzeros;
  delete [] mumps_->a;
  mumps_->a = NULL;
  ~~~

[traversaro]

The fix of setting nnz did not solved the issue, probably because there are other cases of this kind of "optional" arguments that are consider if they are greater than zero.

However the following simpler patch fixed the Valgrind error:
~~~patch
--- coinor-ipopt-3.11.9.orig/Ipopt/src/Algorithm/LinearSolvers/IpMumpsSolverInterface.cpp
+++ coinor-ipopt-3.11.9/Ipopt/src/Algorithm/LinearSolvers/IpMumpsSolverInterface.cpp
@@ -40,6 +40,8 @@
# endif
#endif

+#include
+
namespace Ipopt
{
#if COIN_IPOPT_VERBOSITY > 0
@@ -56,6 +58,8 @@ namespace Ipopt
dbg_verbosity);
//initialize mumps
DMUMPS_STRUC_C* mumps_ = new DMUMPS_STRUC_C;

• std::memset(mumps_, 0, sizeof(DMUMPS_STRUC_C));
  +
  #ifndef MUMPS_MPI_H
  #if defined(HAVE_MPI_INITIALIZED)
  ~~~

If you are interested in the not signed and totally unsupported patched .deb package, see
coinor-libipopt1_3.11.9-2_amd64.deb.zip (just unzip to avoid the GitHub restriction on file types).

[traversaro]

I guess fix of this kind would qualify for a request for SRU for Ubuntu, but I need to double check.

[traversaro]

Recap comment

TL;DR: The ipopt package shipped with Ubuntu Bionic 18.04 has a not initialized memory bug that could cause (with low probability) crashes at runtime.

Tagging a few people that I am aware may be using ipopt in some form and may benefit from knowing this: @jslee02 @chrisdembia @liesrock @fabrizioromanelli @awinkler @neunertm @hongkai-dai @bchretien @jgillis @ghorn .

[traversaro]

Note: the problem is actually due to the use of IPOPT with mumps >= 5.1, so I guess it affects also the formula in https://github.com/dartsim/homebrew-dart @jslee02 and any eventual port of ipopt in the vcpkg fork by @albertziegenhagel .

[traversaro]

Issue fixed in IPOPT upstream in https://projects.coin-or.org/Ipopt/changeset?sfp_email=&sfph_mail=&reponame=&new=2724%40trunk&old=2723%40trunk&sfp_email=&sfph_mail= . I will try to prepare the SRU procedure for Ubuntu 18.04 in my free time, but if anyone is interested in helping on this it would be great. I suspect this would mean waiting for a new release of IPOPT upstream, and than package it in latest Debian. For the short term, providing a patched IPOPT in the robotology repository is probably the way to go.

[traversaro]

The issue was reproduced on Travis with Docker, see https://travis-ci.org/robotology/idyntree/jobs/418929136 .

[traversaro]

IPOPT released a new version 3.12.11 ( https://github.com/coin-or/Ipopt/releases/tag/releases%2F3.12.11 ), that contains the fix for this issue. I guess the next step is updating Debian's IPOPT package.

[traversaro]

Probably getting a recent IPOPT version in Debian in time for the Ubuntu 20.04 sync is a more realistic goal for long term handling of this issue. Unfortunately I personally lack the time to work on this.

[drew-parsons]

Debian is in the the final stages of releasing the next stable version, which makes it tricky though not impossible to update coinor-ipopt in unstable for the time being, though the new version can be placed in experimental. Maybe a small patch for the specific issue can be made for the forthcoming stable release. I'll notify the Debian ipopt maintainer.

Note that MUMPS 5.2.0 is now released, so I'll expect to update mumps in Debian unstable once the new stable release is finished.

[drew-parsons]

Hi Greg Horn and Debian science,

idyntree developers are requesting an upgrade of ipopt (coinor-ipopt) in
Debian (i.e. Ubuntu) to deal with a bug interfacing with MUMPS, see
https://github.com/robotology/idyntree/issues/456

Greg, are you available to work on this upgrade? Can a small patch be
applied to the old version in unstable to fix the version going into
buster?
Should coinor-ipopt be moved to the Debian Science team for team
maintenance?

Drew

On 2019-06-15 06:28, Silvio Traversaro wrote:

Probably getting a recent IPOPT version in Debian in time for the
Ubuntu 20.04 sync is a more realistic goal for long term handling of
this issue. Unfortunately I personally lack the time to work on this.

--
You are receiving this because Drew Parsons cc:d you.
Reply to this email directly, view it on GitHub [1], or mute the
thread [2].

Links:

[1]
https://github.com/robotology/idyntree/issues/456?email_source=notifications&email_token=AGKHYAC6ABPKPU3SKXGHVFDP2P5NRA5CNFSM4FF6VNXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXX4GYQ#issuecomment-502252386
[2]
https://github.com/notifications/unsubscribe-auth/AGKHYADLSWAHNBSUFZ5M4GTP2P5NRANCNFSM4FF6VNXA

[ginggs]

Would someone be able to file a bug against the coinor-ipopt package in Debian? Including a reproducer and a patch (is https://github.com/robotology/idyntree/issues/456#issuecomment-399683933 all that is needed?) would be of great help.
If it's easier to file the bug in Launchpad, do that, and I will copy it over to the Debian BTS.

Once we have a fix in Debian unstable, I can help with sponsoring an SRU for Ubuntu 18.04.

[drew-parsons]

Debian Bug#929265 was already filed for the upgrade, but I've filed
Debian Bug#930577 for this specific bug.

[drew-parsons]

@ghorn Hi Greg. idyntree needs ipopt patched in Debian (ubuntu). Are you still maintaining the Debian package coiner-ipopt?

[traversaro]

Thanks a lot @ginggs and @RizzerOnGitHub !
Just as a reference, the commit that solved the problem in upstream is https://github.com/coin-or/Ipopt/commit/4c36f888f1e8a609975f0bee60fe04958024236c . The GitHub reference in the commit name is wrong because since then the coin-or/Ipopt repo has been replaced with the one migrated from trac, and all the "old" PR and issue got lost.

[traversaro]

A simple self-contained example to reproduce the issue is contained in https://github.com/traversaro/ipopt-cmake-demo .
To reproduce the issue, run:
~
sudo apt install cmake valgrind build-essentials
git clone https://github.com/traversaro/ipopt-cmake-demo
cd ipopt-cmake-demo
mkdir build
cd build
cmake ..
make
valgrind ./ipopt_example
~
The first output of valgrind will be:
~
==15450== Conditional jump or move depends on uninitialised value(s)
==15450== at 0x67F5FD7: dmumps_ (in /usr/lib/x86_64-linux-gnu/libdmumps_seq-5.1.2.so)
==15450== by 0x67FB000: dmumps_f77_ (in /usr/lib/x86_64-linux-gnu/libdmumps_seq-5.1.2.so)
==15450== by 0x67F3EE1: dmumps_c (in /usr/lib/x86_64-linux-gnu/libdmumps_seq-5.1.2.so)
==15450== by 0x4FC4C86: Ipopt::MumpsSolverInterface::MumpsSolverInterface() (in /usr/lib/libipopt.so.1.9.9)
==15450== by 0x4EDE07A: Ipopt::AlgorithmBuilder::BuildBasicAlgorithm(Ipopt::Journalist const&, Ipopt::OptionsList const&, std::__cxx11::basic_string, std::allocator > const&) (in /usr/lib/libipopt.so.1.9.9)
==15450== by 0x4E97175: Ipopt::IpoptApplication::OptimizeNLP(Ipopt::SmartPtr const&, Ipopt::SmartPtr&) (in /usr/lib/libipopt.so.1.9.9)
==15450== by 0x4E913B8: Ipopt::IpoptApplication::OptimizeNLP(Ipopt::SmartPtr const&) (in /usr/lib/libipopt.so.1.9.9)
==15450== by 0x4E93679: Ipopt::IpoptApplication::OptimizeTNLP(Ipopt::SmartPtr const&) (in /usr/lib/libipopt.so.1.9.9)
==15450== by 0x10AD15: main (in /home/straversaro/src/ipopt-cmake-demo/build/ipopt_example)
==15450==
~

[drew-parsons]

Confirmed the problem is fixed in version 3.11.9-2.2 in Debian unstable. Thanks @traversaro and @ginggs

[traversaro]

SRU bug in Ubuntu : https://bugs.launchpad.net/ubuntu/bionic/+source/coinor-ipopt/+bug/1833037 (thanks @ginggs!).

[ginggs]

I've uploaded the SRU packages and they should now be available in disco-proposed, cosmic-proposed and bionic-proposed. It will be much appreciated if someone would help by testing the packages. The procedure is described in the SRU bug comments.

[traversaro]

Thanks a lot @ginggs . I verified that the fix is indeed working in bionic (see the SRU bug comment).

[traversaro]

For Ubuntu 18.04, the package has been now published on bionic-updates channel (see https://packages.ubuntu.com/source/bionic-updates/coinor-ipopt). The problem discussed in this has been solved upstream, and has been solved on the currently most used OS on which iDynTree is used (Ubuntu 18.04). Depending on the used IPOPT and MUMPS version, the problem can arise on some systems or configurations, but also in those case the proper fix will be to either update IPOPT or add to it the hotfix described before. Having said that, I think the issue in iDynTree can be closed, as soon as we enable again the CI Valgrind tests on Bionic. Many thanks to everyone that contributed in spotting and solving this issue: @S-Dafarra @svigerske @RizzerOnGitHub @ginggs .

[traversaro]

Valgrind test in Bionic re-enabled in https://github.com/robotology/idyntree/issues/456 .