Merge pull request #2048 from ERGO-Code/fix-2047

Added some sanity checks to `Highs::writeLocalModel` to prevent segfaults if called directly by a user

check/TestFilereader.cpp

@@ -11,6 +11,7 @@
 #include "lp_data/HighsLpUtils.h"
 
 const bool dev_run = false;
+const double inf = kHighsInf;
 
 TEST_CASE("filereader-edge-cases", "[highs_filereader]") {
   std::string model = "";
@@ -357,3 +358,61 @@ TEST_CASE("filereader-dD2e", "[highs_filereader]") {
 //   double objective_value = highs.getInfo().objective_function_value;
 //   REQUIRE(objective_value == optimal_objective_value);
 // }
+
+TEST_CASE("writeLocalModel", "[highs_filereader]") {
+  Highs h;
+  h.setOptionValue("output_flag", dev_run);
+  std::string write_model_file = "foo.mps";
+  HighsModel model;
+  HighsLp& lp = model.lp_;
+  ;
+  lp.num_col_ = 2;
+  lp.num_row_ = 3;
+  lp.col_cost_ = {8, 10};
+  lp.row_lower_ = {7, 12, 6};
+  lp.row_upper_ = {inf, inf, inf};
+  lp.a_matrix_.start_ = {0, 3, 6};
+  lp.a_matrix_.index_ = {0, 1, 2, 0, 1, 2};
+  lp.a_matrix_.value_ = {2, 3, 2, 2, 4, 1};
+
+  if (dev_run) printf("\nModel with no column lower or upper bounds\n");
+  REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kError);
+  lp.col_lower_ = {0, 0};
+
+  if (dev_run) printf("\nModel with no column upper bounds\n");
+  REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kError);
+  lp.col_upper_ = {inf, inf};
+
+  // Model has no dimensions for a_matrix_, but these are set in
+  // writeLocalModel.
+  if (dev_run) printf("\nModel with no column or row names\n");
+  REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kWarning);
+  lp.col_names_ = {"C0", "C1"};
+  lp.row_names_ = {"R0", "R1", "R2"};
+
+  if (dev_run) printf("\nModel with column and row names\n");
+  REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kOk);
+
+  // Introduce illegal start
+  if (dev_run) printf("\nModel with start entry > num_nz\n");
+  lp.a_matrix_.start_ = {0, 7, 6};
+  REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kError);
+
+  // Introduce illegal start
+  if (dev_run) printf("\nModel with start entry -1\n");
+  lp.a_matrix_.start_ = {0, -1, 6};
+  REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kError);
+  lp.a_matrix_.start_ = {0, 3, 6};
+
+  // Introduce illegal index
+  if (dev_run) printf("\nModel with index entry -1\n");
+  lp.a_matrix_.index_ = {0, -1, 2, 0, 1, 2};
+  REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kError);
+
+  // Introduce illegal index
+  if (dev_run) printf("\nModel with index entry 3 >= num_row\n");
+  lp.a_matrix_.index_ = {0, 1, 3, 0, 1, 2};
+  REQUIRE(h.writeLocalModel(model, write_model_file) == HighsStatus::kError);
+
+  std::remove(write_model_file.c_str());
+}

src/lp_data/Highs.cpp

@@ -705,9 +705,32 @@ HighsStatus Highs::writePresolvedModel(const std::string& filename) {
 HighsStatus Highs::writeLocalModel(HighsModel& model,
                                    const std::string& filename) {
   HighsStatus return_status = HighsStatus::kOk;
+  HighsStatus call_status;
+
+  HighsLp& lp = model.lp_;
+  // Dimensions in a_matrix_ may not be set, so take them from lp
+  lp.setMatrixDimensions();
 
   // Ensure that the LP is column-wise
-  model.lp_.ensureColwise();
+  lp.ensureColwise();
+
+  // Ensure that the dimensions are OK
+  if (!lpDimensionsOk("writeLocalModel", lp, options_.log_options))
+    return HighsStatus::kError;
+
+  if (model.hessian_.dim_ > 0) {
+    call_status = assessHessianDimensions(options_, model.hessian_);
+    if (call_status == HighsStatus::kError) return call_status;
+  }
+
+  // Check that the matrix starts are OK
+  call_status = lp.a_matrix_.assessStart(options_.log_options);
+  if (call_status == HighsStatus::kError) return call_status;
+
+  // Check that the matrix indices are within bounds
+  call_status = lp.a_matrix_.assessIndexBounds(options_.log_options);
+  if (call_status == HighsStatus::kError) return call_status;
+
   // Check for repeated column or row names that would corrupt the file
   if (model.lp_.col_hash_.hasDuplicate(model.lp_.col_names_)) {
     highsLogUser(options_.log_options, HighsLogType::kError,

src/lp_data/HighsLpUtils.cpp

@@ -130,7 +130,7 @@ bool lpDimensionsOk(std::string message, const HighsLp& lp,
   HighsInt col_upper_size = lp.col_upper_.size();
   bool legal_col_cost_size = col_cost_size >= num_col;
   bool legal_col_lower_size = col_lower_size >= num_col;
-  bool legal_col_upper_size = col_lower_size >= num_col;
+  bool legal_col_upper_size = col_upper_size >= num_col;
   if (!legal_col_cost_size)
     highsLogUser(log_options, HighsLogType::kError,
                  "LP dimension validation (%s) fails on col_cost.size() = %d < "

src/model/HighsHessianUtils.cpp

@@ -28,14 +28,9 @@ HighsStatus assessHessian(HighsHessian& hessian, const HighsOptions& options) {
   HighsStatus return_status = HighsStatus::kOk;
   HighsStatus call_status;
 
-  // Assess the Hessian dimensions and vector sizes, returning on error
-  vector<HighsInt> hessian_p_end;
-  const bool partitioned = false;
-  call_status = assessMatrixDimensions(
-      options.log_options, hessian.dim_, partitioned, hessian.start_,
-      hessian_p_end, hessian.index_, hessian.value_);
-  return_status = interpretCallStatus(options.log_options, call_status,
-                                      return_status, "assessMatrixDimensions");
+  return_status = interpretCallStatus(options.log_options,
+                                      assessHessianDimensions(options, hessian),
+                                      return_status, "assessHessianDimensions");
   if (return_status == HighsStatus::kError) return return_status;
 
   // If the Hessian has no columns there is nothing left to test
@@ -110,6 +105,18 @@ HighsStatus assessHessian(HighsHessian& hessian, const HighsOptions& options) {
   return return_status;
 }
 
+HighsStatus assessHessianDimensions(const HighsOptions& options,
+                                    HighsHessian& hessian) {
+  if (hessian.dim_ == 0) return HighsStatus::kOk;
+
+  // Assess the Hessian dimensions and vector sizes
+  vector<HighsInt> hessian_p_end;
+  const bool partitioned = false;
+  return assessMatrixDimensions(options.log_options, hessian.dim_, partitioned,
+                                hessian.start_, hessian_p_end, hessian.index_,
+                                hessian.value_);
+}
+
 void completeHessianDiagonal(const HighsOptions& options,
                              HighsHessian& hessian) {
   // Count the number of missing diagonal entries

src/util/HighsMatrixUtils.cpp

@@ -68,7 +68,7 @@ HighsStatus assessMatrix(
   //
   // Check whether the first start is zero
   if (matrix_start[0]) {
-    highsLogUser(log_options, HighsLogType::kWarning,
+    highsLogUser(log_options, HighsLogType::kError,
                  "%s matrix start vector begins with %" HIGHSINT_FORMAT
                  " rather than 0\n",
                  matrix_name.c_str(), matrix_start[0]);

src/util/HighsSparseMatrix.cpp

@@ -722,6 +722,65 @@ void HighsSparseMatrix::deleteRows(
   this->num_row_ = new_num_row;
 }
 
+HighsStatus HighsSparseMatrix::assessStart(const HighsLogOptions& log_options) {
+  // Identify main dimensions
+  HighsInt vec_dim;
+  HighsInt num_vec;
+  if (this->isColwise()) {
+    vec_dim = this->num_row_;
+    num_vec = this->num_col_;
+  } else {
+    vec_dim = this->num_col_;
+    num_vec = this->num_row_;
+  }
+  if (this->start_[0]) {
+    highsLogUser(log_options, HighsLogType::kError,
+                 "Matrix start[0] = %d, not 0\n", int(this->start_[0]));
+    return HighsStatus::kError;
+  }
+  HighsInt num_nz = this->numNz();
+  for (HighsInt iVec = 1; iVec < num_vec; iVec++) {
+    if (this->start_[iVec] < this->start_[iVec - 1]) {
+      highsLogUser(log_options, HighsLogType::kError,
+                   "Matrix start[%d] = %d > %d = start[%d]\n", int(iVec),
+                   int(this->start_[iVec]), int(this->start_[iVec - 1]),
+                   int(iVec - 1));
+      return HighsStatus::kError;
+    }
+    if (this->start_[iVec] > num_nz) {
+      highsLogUser(log_options, HighsLogType::kError,
+                   "Matrix start[%d] = %d > %d = number of nonzeros\n",
+                   int(iVec), int(this->start_[iVec]), int(num_nz));
+      return HighsStatus::kError;
+    }
+  }
+  return HighsStatus::kOk;
+}
+
+HighsStatus HighsSparseMatrix::assessIndexBounds(
+    const HighsLogOptions& log_options) {
+  // Identify main dimensions
+  HighsInt vec_dim;
+  HighsInt num_vec;
+  if (this->isColwise()) {
+    vec_dim = this->num_row_;
+    //    num_vec = this->num_col_;
+  } else {
+    vec_dim = this->num_col_;
+    //    num_vec = this->num_row_;
+  }
+  HighsInt num_nz = this->numNz();
+  for (HighsInt iEl = 1; iEl < num_nz; iEl++) {
+    if (this->index_[iEl] < 0 || this->index_[iEl] >= vec_dim) {
+      highsLogUser(log_options, HighsLogType::kError,
+                   "Matrix index[%d] = %d is not in legal range of [0, %d)\n",
+                   int(iEl), int(this->index_[iEl]), vec_dim);
+      return HighsStatus::kError;
+    }
+  }
+  return HighsStatus::kOk;
+}
+
 HighsStatus HighsSparseMatrix::assess(const HighsLogOptions& log_options,
                                       const std::string matrix_name,
                                       const double small_matrix_value,

src/util/HighsSparseMatrix.h

@@ -66,6 +66,9 @@ class HighsSparseMatrix {
   void deleteRows(const HighsIndexCollection& index_collection);
   HighsStatus assessDimensions(const HighsLogOptions& log_options,
                                const std::string matrix_name);
+  HighsStatus assessStart(const HighsLogOptions& log_options);
+  HighsStatus assessIndexBounds(const HighsLogOptions& log_options);
+
   HighsStatus assess(const HighsLogOptions& log_options,
                      const std::string matrix_name,
                      const double small_matrix_value,