Skip to content

Commit

Permalink
Fix potential double free in vpMatrix and vpArray
Browse files Browse the repository at this point in the history
  • Loading branch information
fspindle committed Mar 13, 2024
1 parent b1c4c58 commit 8f0c01c
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 27 deletions.
8 changes: 0 additions & 8 deletions example/math/quadprog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,19 +105,15 @@ int main(int argc, char **argv)
r = randV(o) * 5;
C = randM(p, n) * 5;

std::cout << "DEBUG 1 before solveBySVD()" << std::endl;
// make sure Cx <= d has a solution within Ax = b
vpColVector x = A.solveBySVD(b);
std::cout << "DEBUG 2 after solveBySVD()" << std::endl;
d = C * x;
for (int i = 0; i < p; ++i)
d[i] += (5. * rand()) / RAND_MAX;

// solver with warm start
vpQuadProg qp_WS;

std::cout << "DEBUG 3 after vpQuadProg()" << std::endl;

// timing
int total = 100;
double t_WS(0), t_noWS(0);
Expand Down Expand Up @@ -146,9 +142,7 @@ int main(int argc, char **argv)
vpQuadProg qp;
x = 0;
double t = vpTime::measureTimeMs();
std::cout << "DEBUG 4 before solveQP()" << std::endl;
qp.solveQP(Q, r, A, b, C, d, x);
std::cout << "DEBUG 4 after solveQP()" << std::endl;

t_noWS += vpTime::measureTimeMs() - t;
#ifdef VISP_HAVE_DISPLAY
Expand All @@ -159,9 +153,7 @@ int main(int argc, char **argv)
// with warm start
x = 0;
t = vpTime::measureTimeMs();
std::cout << "DEBUG 5 before solveQP()" << std::endl;
qp_WS.solveQP(Q, r, A, b, C, d, x);
std::cout << "DEBUG 5 after solveQP()" << std::endl;

t_WS += vpTime::measureTimeMs() - t;
#ifdef VISP_HAVE_DISPLAY
Expand Down
13 changes: 0 additions & 13 deletions example/math/quadprog_eq.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,22 +106,17 @@ int main(int argc, char **argv)

// make sure Cx <= d has a solution within Ax = b

std::cout << "DEBUG 1 before solveBySVD()" << std::endl;
vpColVector x = A.solveBySVD(b);
std::cout << "DEBUG 1 after solveBySVD()" << std::endl;
d = C * x;
for (int i = 0; i < p; ++i)
d[i] += (5. * rand()) / RAND_MAX;

// solver with stored equality and warm start
vpQuadProg qp_WS;
std::cout << "DEBUG 2 before setEqualityConstraint()" << std::endl;
qp_WS.setEqualityConstraint(A, b);
std::cout << "DEBUG 2 after setEqualityConstraint()" << std::endl;

vpQuadProg qp_ineq_WS;
qp_ineq_WS.setEqualityConstraint(A, b);
std::cout << "DEBUG 3 after setEqualityConstraint()" << std::endl;

// timing
int total = 100;
Expand All @@ -146,9 +141,7 @@ int main(int argc, char **argv)
// without warm start
x = 0;
double t = vpTime::measureTimeMs();
std::cout << "DEBUG 4 before solveQPe()" << std::endl;
vpQuadProg::solveQPe(Q, r, A, b, x);
std::cout << "DEBUG 4 after solveQPe()" << std::endl;

t_noWS += vpTime::measureTimeMs() - t;
#ifdef VISP_HAVE_DISPLAY
Expand All @@ -159,9 +152,7 @@ int main(int argc, char **argv)
// with pre-solved Ax = b
x = 0;
t = vpTime::measureTimeMs();
std::cout << "DEBUG 5 before solveQPe()" << std::endl;
qp_WS.solveQPe(Q, r, x);
std::cout << "DEBUG 5 after solveQPe()" << std::endl;

t_WS += vpTime::measureTimeMs() - t;
#ifdef VISP_HAVE_DISPLAY
Expand All @@ -174,9 +165,7 @@ int main(int argc, char **argv)
x = 0;
vpQuadProg qp;
t = vpTime::measureTimeMs();
std::cout << "DEBUG 6 before solveQP()" << std::endl;
qp.solveQP(Q, r, A, b, C, d, x);
std::cout << "DEBUG 6 after solveQP()" << std::endl;

t_ineq_noWS += vpTime::measureTimeMs() - t;
#ifdef VISP_HAVE_DISPLAY
Expand All @@ -187,9 +176,7 @@ int main(int argc, char **argv)
// with warm start + pre-solving
x = 0;
t = vpTime::measureTimeMs();
std::cout << "DEBUG 7 before solveQPi()" << std::endl;
qp_ineq_WS.solveQPi(Q, r, C, d, x, true);
std::cout << "DEBUG 7 after solveQPi()" << std::endl;

t_ineq_WS += vpTime::measureTimeMs() - t;
#ifdef VISP_HAVE_DISPLAY
Expand Down
18 changes: 14 additions & 4 deletions modules/core/include/visp3/core/vpArray2D.h
Original file line number Diff line number Diff line change
Expand Up @@ -320,10 +320,13 @@ template <class Type> class vpArray2D

// Reallocation of this->data array
this->dsize = nrows * ncols;
Type *tmp_data = (Type *)realloc(this->data, this->dsize * sizeof(Type));
Type *tmp_data = reinterpret_cast<Type *>(realloc(this->data, this->dsize * sizeof(Type)));
if (tmp_data) {
this->data = tmp_data;
}
else {
this->data = nullptr;
}

if ((nullptr == this->data) && (0 != this->dsize)) {
if (copyTmp != nullptr) {
Expand All @@ -332,10 +335,13 @@ template <class Type> class vpArray2D
throw(vpException(vpException::memoryAllocationError, "Memory allocation error when allocating 2D array data"));
}

Type **tmp_rowPtrs = (Type **)realloc(this->rowPtrs, nrows * sizeof(Type *));
Type **tmp_rowPtrs = reinterpret_cast<Type **>(realloc(this->rowPtrs, nrows * sizeof(Type *)));
if (tmp_rowPtrs) {
this->rowPtrs = tmp_rowPtrs;
}
else {
this->rowPtrs = nullptr;
}
if ((nullptr == this->rowPtrs) && (0 != this->dsize)) {
if (copyTmp != nullptr) {
delete[] copyTmp;
Expand Down Expand Up @@ -472,8 +478,12 @@ template <class Type> class vpArray2D
vpArray2D<Type> &operator=(vpArray2D<Type> &&other) noexcept
{
if (this != &other) {
free(data);
free(rowPtrs);
if (data) {
free(data);
}
if (rowPtrs) {
free(rowPtrs);
}

rowNum = other.rowNum;
colNum = other.colNum;
Expand Down
8 changes: 6 additions & 2 deletions modules/core/src/math/matrix/vpMatrix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -681,8 +681,12 @@ vpMatrix &vpMatrix::operator=(const vpMatrix &A)
vpMatrix &vpMatrix::operator=(vpMatrix &&other)
{
if (this != &other) {
free(data);
free(rowPtrs);
if (data) {
free(data);
}
if (rowPtrs) {
free(rowPtrs);
}

rowNum = other.rowNum;
colNum = other.colNum;
Expand Down
45 changes: 45 additions & 0 deletions modules/core/test/math/testMatrix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,29 @@

namespace
{
bool test_memory(unsigned int nrows, unsigned int ncols, const vpMatrix &M, const std::string &matrix_name, bool pointer_is_null)
{
if (pointer_is_null) {
if (M.data) {
std::cerr << "Wrong data pointer (" << M.data << ") in matrix " << matrix_name << ": should be null" << std::endl;
}
}
else {
if (!M.data) {
std::cerr << "Wrong data pointer (" << M.data << ") in matrix " << matrix_name << ": should be non null" << std::endl;
return false;
}
}

if (M.getRows() != nrows || M.getCols() != ncols) {
std::cerr << "Wrong matrix " << matrix_name << "(" << nrows << ", " << ncols << " size: "
<< M.getRows() << " x " << M.getCols() << std::endl;
return false;
}
std::cout << "Test matrix " << matrix_name << " succeed" << std::endl;
return true;
}

bool test(const std::string &s, const vpMatrix &M, const std::vector<double> &bench)
{
static unsigned int cpt = 0;
Expand Down Expand Up @@ -122,6 +145,28 @@ int main(int argc, char *argv[])
}
}

{
unsigned int nrows = 2, ncols = 3;
vpMatrix A(nrows, ncols);
if (test_memory(nrows, ncols, A, "A", false) == false) {
return EXIT_FAILURE;
}
vpMatrix B, C;
if (test_memory(0, 0, B, "B", true) == false) {
return EXIT_FAILURE;
}
if (test_memory(0, 0, C, "C", true)== false) {
return EXIT_FAILURE;
}
B = A;
if (test_memory(nrows, ncols, B, "B", false)== false) {
return EXIT_FAILURE;
}
B = C;
if (test_memory(0, 0, C, "C", true)== false) {
return EXIT_FAILURE;
}
}
{
const double val = 10.0;
vpMatrix M, M2(5, 5, val);
Expand Down

0 comments on commit 8f0c01c

Please sign in to comment.