Big bunch of fixes, including memory leaks, bad code pathes, insufficent error reporting, uninitialized variables, etc, etc.

Edited sparse/static_sparse_matrix.h, added an internal state enum to represent errors and the like.

src/parser/read_lab_file.cpp

@@ -53,7 +53,7 @@ mrmc::dtmc::labeling * read_lab_file(int node_count, const char * filename)
    FILE *P;
 
    //TODO (Thomas Heinemann): handle files with lines that are longer than BUFFER_SIZE
-   //TODO (Thomas Heinemann): Throw errors if fgets return NULL in the declaration.
+   //TODO (Thomas Heinemann): Throw errors if fgets return NULL in the declaration. (fixed by Philipp. Or?)
 
    char s[BUFFER_SIZE];              //String buffer
    char *saveptr = NULL;             //Buffer for strtok_r
@@ -64,14 +64,14 @@ mrmc::dtmc::labeling * read_lab_file(int node_count, const char * filename)
    if (P == NULL) {
       pantheios::log_ERROR("File could not be opened.");
       throw mrmc::exceptions::file_IO_exception();
+	  return NULL;
    }
 
-   if (fgets(s, BUFFER_SIZE, P)) {
-      if (strcmp(s, "#DECLARATION\n")) {
+   if (!fgets(s, BUFFER_SIZE, P) || strcmp(s, "#DECLARATION\n")) {
 		fclose(P);
 		pantheios::log_ERROR("Wrong declaration section (\"#DECLARATION\" missing).");
 		throw mrmc::exceptions::wrong_file_format();
-      }
+		return NULL;
    }
 
 
@@ -104,6 +104,8 @@ mrmc::dtmc::labeling * read_lab_file(int node_count, const char * filename)
       } else {
          pantheios::log_ERROR("EOF in the declaration section");
          throw mrmc::exceptions::wrong_file_format();
+		 delete[] decl_buffer;
+		 return NULL;
       }
    } while (need_next_iteration);
 
@@ -132,17 +134,18 @@ mrmc::dtmc::labeling * read_lab_file(int node_count, const char * filename)
       result -> addProposition(proposition);
    }
 
+   // Free decl_buffer
+   delete[] decl_buffer;
 
 
    saveptr = NULL;                        //resetting save pointer for strtok_r
 
-   if (fgets(s, BUFFER_SIZE, P)) {
-      if (strcmp(s, "#END\n")) {
+   if (!fgets(s, BUFFER_SIZE, P) || strcmp(s, "#END\n")) {
 		fclose(P);
 		delete result;
 		pantheios::log_ERROR("Wrong declaration section (\"#END\" missing).");
 		throw mrmc::exceptions::wrong_file_format();
-      }
+		return NULL;
    }
 
    while (fgets(s, BUFFER_SIZE, P)) {
@@ -157,6 +160,7 @@ mrmc::dtmc::labeling * read_lab_file(int node_count, const char * filename)
          delete result;
          pantheios::log_ERROR("Line assigning propositions does not start with a node number.");
          throw mrmc::exceptions::wrong_file_format();
+		 return NULL;
       }
       do {
          token = STRTOK_FUNC(NULL, sep, &saveptr);
@@ -169,9 +173,7 @@ mrmc::dtmc::labeling * read_lab_file(int node_count, const char * filename)
    }
 
    fclose(P);
-
    return result;
-
 }
 
 } //namespace parser

src/parser/read_tra_file.cpp

@@ -102,6 +102,7 @@ sparse::StaticSparseMatrix<double> * read_tra_file(const char * filename) {
 	if(p == NULL) {
 		pantheios::log_ERROR("File ", filename, " was not readable (Does it exist?)");
 		throw exceptions::file_IO_exception("mrmc::read_tra_file: Error opening file! (Does it exist?)");
+		return NULL;
 	}
 	non_zero = make_first_pass(p);
 
@@ -109,24 +110,22 @@ sparse::StaticSparseMatrix<double> * read_tra_file(const char * filename) {
 	rewind(p);
 
 	//Reading No. of states
-   if (fgets(s, BUFFER_SIZE, p) != NULL) {
-      if (sscanf( s, "STATES %d", &rows) == 0) {
+	if ((fgets(s, BUFFER_SIZE, p) == NULL) || (sscanf(s, "STATES %d", &rows) == 0)) {
 		pantheios::log_WARNING(pantheios::integer(rows));
 		(void)fclose(p);
 		throw mrmc::exceptions::wrong_file_format();
-      }
+		return NULL;
 	}
 
 	/* Reading No. of transitions
 	 * Note that the result is not used in this function as make_first_pass()
 	 * computes the relevant number (non_zero)
 	 */
-   if (fgets(s, BUFFER_SIZE, p) != NULL) {
 	uint_fast32_t nnz=0;
-      if (sscanf( s, "TRANSITIONS %d", &nnz) == 0) {
+	if ((fgets(s, BUFFER_SIZE, p) == NULL) || (sscanf(s, "TRANSITIONS %d", &nnz) == 0)) {
 		(void)fclose(p);
 		throw mrmc::exceptions::wrong_file_format();
-      }
+		return NULL;
 	}
 
    pantheios::log_DEBUG("Creating matrix with ",
@@ -137,21 +136,22 @@ sparse::StaticSparseMatrix<double> * read_tra_file(const char * filename) {
 	 * non-zero elements has to be specified (which is non_zero, computed by make_first_pass)
 	 */
 	sp = new sparse::StaticSparseMatrix<double>(rows,non_zero);
-   sp->initialize();
 	if ( NULL == sp ) {
 		throw std::bad_alloc();
 		return NULL;
 	}
+	sp->initialize();
 
 	//Reading transitions (one per line) and saving the results in the matrix
-   while (NULL != fgets( s, BUFFER_SIZE, p ))
-   {
+	while (NULL != fgets(s, BUFFER_SIZE, p )) {
 		uint_fast32_t row=0, col=0;
 		double val = 0.0;
 		if (sscanf(s, "%d%d%lf", &row, &col, &val) != 3) {
-         delete sp;
 			(void)fclose(p);
 			throw mrmc::exceptions::wrong_file_format();
+			// Delete Matrix to free allocated memory
+			delete sp;
+			return NULL;
 		}
 		pantheios::log_DEBUG("Write value ",
 							pantheios::real(val),

src/sparse/static_sparse_matrix.h

@@ -26,6 +26,23 @@ namespace sparse {
 template <class T>
 class StaticSparseMatrix {
  public:
+	//! Status enum
+	/*!
+		An Enum representing the internal state of the Matrix. 
+		After creating the Matrix using the Constructor, the Object is in state UnInitialized. After calling initialize(), that state changes to Initialized and after all entries have been entered and finalize() has been called, to ReadReady.
+		Should a critical error occur in any of the former functions, the state will change to Error.
+		@see getState()
+		@see isReadReady()
+		@see isInitialized()
+		@see hasError()
+	 */
+	 enum MatrixStatus {
+		Error = -1,
+		UnInitialized = 0,
+		Initialized = 1,
+		ReadReady = 2,
+	 };
+
 
 	//! Constructor
 	 /*!
@@ -33,6 +50,7 @@ class StaticSparseMatrix {
 		\param non_zero_entries The exact count of entries that will be submitted through addNextValue *excluding* those on the diagonal (A_{i,j} with i = j)
 	 */
 	StaticSparseMatrix(uint_fast32_t rows, uint_fast32_t non_zero_entries) {
+		setState(MatrixStatus::UnInitialized);
 		current_size = 0;
 		storage_size = 0;
 		
@@ -48,6 +66,7 @@ class StaticSparseMatrix {
 	}
 
 	~StaticSparseMatrix() {
+		setState(MatrixStatus::UnInitialized);
 		if (value_storage != NULL) {
 			//free(value_storage);
 			delete[] value_storage;
@@ -68,7 +87,7 @@ class StaticSparseMatrix {
 
 	//! Getter for saving matrix entry A_{row,col} to target
 	/*!
-		Getter function for the matrix. 
+		Getter function for the matrix. This function does not check the internal status for errors for performance reasons.
 		\param row 1-based index of the requested row
 		\param col 1-based index of the requested column
 		\param target pointer to where the result will be stored
@@ -109,17 +128,19 @@ class StaticSparseMatrix {
 		Mandatory initialization of the matrix, must be called before using any other member function.
 	 */
 	void initialize() {
-
-		if (row_count == 0) {
+		if (internal_status != MatrixStatus::UnInitialized) {
+			pantheios::log_ERROR("StaticSparseMatrix::initialize: Throwing invalid state for status flag != 0 (is ", pantheios::integer(internal_status)," - Already initialized?");
+			throw mrmc::exceptions::invalid_state("StaticSparseMatrix::initialize: Invalid state for status flag != 0 - Already initialized?");
+			triggerErrorState();
+		} else if (row_count == 0) {
 			pantheios::log_ERROR("StaticSparseMatrix::initialize: Throwing invalid_argument for row_count = 0");
 			throw mrmc::exceptions::invalid_argument("mrmc::StaticSparseMatrix::initialize: Matrix with 0 rows is not reasonable");
-		}
-
-		if (((row_count * row_count) - row_count) < non_zero_entry_count) {
+			triggerErrorState();
+		} else if (((row_count * row_count) - row_count) < non_zero_entry_count) {
 			pantheios::log_ERROR("StaticSparseMatrix::initialize: Throwing invalid_argument: More non-zero entries than entries in target matrix");
 			throw mrmc::exceptions::invalid_argument("mrmc::StaticSparseMatrix::initialize: More non-zero entries than entries in target matrix");
-		}
-
+			triggerErrorState();
+		} else {
 			storage_size = non_zero_entry_count;
 			last_row = 0;
 
@@ -139,6 +160,10 @@ class StaticSparseMatrix {
 			if ((value_storage == NULL) || (column_indications == NULL) || (row_indications == NULL) || (diagonal_storage == NULL)) {
 				pantheios::log_ERROR("StaticSparseMatrix::initialize: Throwing bad_alloc: memory allocation failed");
 				throw std::bad_alloc();
+				triggerErrorState();
+			} else {
+				setState(MatrixStatus::Initialized);
+			}
 		}
 	}
 
@@ -151,6 +176,7 @@ class StaticSparseMatrix {
 		if ((row > row_count) || (col > row_count) || (row == 0) || (col == 0)) {
 			pantheios::log_ERROR("StaticSparseMatrix::addNextValue: Throwing out_of_range: row or col not in 1 .. rows");
 			throw mrmc::exceptions::out_of_range("mrmc::StaticSparseMatrix::addNextValue: row or col not in 1 .. rows");
+			triggerErrorState();
 		}
 		
 		if (row == col) {
@@ -172,11 +198,15 @@ class StaticSparseMatrix {
 	}
 
 	void finalize() {
-		if (storage_size != current_size) {
+		if (!isInitialized()) {
+			pantheios::log_ERROR("StaticSparseMatrix::finalize: Throwing invalid state for internal state not Initialized (is ", pantheios::integer(internal_status)," - Already finalized?");
+			throw mrmc::exceptions::invalid_state("StaticSparseMatrix::finalize: Invalid state for internal state not Initialized - Already finalized?");
+			triggerErrorState();
+		} else if (storage_size != current_size) {
 			pantheios::log_ERROR("StaticSparseMatrix::finalize: Throwing invalid_state: Wrong call count for addNextValue");
 			throw mrmc::exceptions::invalid_state("mrmc::StaticSparseMatrix::finalize: Wrong call count for addNextValue");
-		}
-
+			triggerErrorState();
+		} else {
 			if (last_row != row_count) {
 				for (uint_fast32_t i = last_row; i < row_count; ++i) {
 					row_indications[i] = current_size;
@@ -184,6 +214,9 @@ class StaticSparseMatrix {
 			}
 
 			row_indications[row_count] = storage_size;
+
+			setState(MatrixStatus::ReadReady);
+		}
 	}
 
 	uint_fast32_t getRowCount() const {
@@ -194,7 +227,30 @@ class StaticSparseMatrix {
 		return value_storage;
 	}
 
+	bool isReadReady() {
+		return (internal_status == MatrixStatus::ReadReady);
+	}
+
+	bool isInitialized() {
+		return (internal_status == MatrixStatus::Initialized || internal_status == MatrixStatus::ReadReady);
+	}
+
+	MatrixStatus getState() {
+		return internal_status;
+	}
+
+	bool hasError() {
+		return (internal_status == MatrixStatus::Error);
+	}
+
 	Eigen::SparseMatrix<T> toEigenSparseMatrix() {
+		Eigen::SparseMatrix<T> mat(row_count, row_count);
+
+		if (!isReadReady()) {
+			pantheios::log_ERROR("StaticSparseMatrix::toEigenSparseMatrix: Throwing invalid state for internal state not ReadReady (is ", pantheios::integer(internal_status),").");
+			throw mrmc::exceptions::invalid_state("StaticSparseMatrix::toEigenSparseMatrix: Invalid state for internal state not ReadReady.");
+			triggerErrorState();
+		} else {
 			typedef Eigen::Triplet<double> ETd;
 			std::vector<ETd> tripletList;
 			tripletList.reserve(non_zero_entry_count + row_count);
@@ -213,9 +269,9 @@ class StaticSparseMatrix {
 				tripletList.push_back(ETd(i, i, diagonal_storage[i]));
 			}
 
-		Eigen::SparseMatrix<T> mat(row_count, row_count);
 			mat.setFromTriplets(tripletList.begin(), tripletList.end());
 			mat.makeCompressed();
+		}
 		
 		return mat;
 	}
@@ -237,6 +293,22 @@ class StaticSparseMatrix {
 	uint_fast32_t* column_indications;
 	/*! Array containing the row boundaries of valueStorage */
 	uint_fast32_t* row_indications;
+
+	/*! Internal status enum, 0 for constructed, 1 for initialized and 2 for finalized, -1 on errors */
+	MatrixStatus internal_status;
+
+	/*! Sets the internal status to Error */
+	void triggerErrorState() {
+		setState(MatrixStatus::Error);
+	}
+
+	/*! 
+		Sets the internal status to new_state iff the current state is not the Error state.
+		@param new_state the new state to be switched to
+	*/
+	void setState(const MatrixStatus new_state) {
+		internal_status = (internal_status == MatrixStatus::Error) ? internal_status : new_state;
+	}
 };
 
 } // namespace sparse

test/parser/read_lab_file_test.cpp

@@ -19,7 +19,7 @@ TEST(ReadLabFileTest, NonExistingFileTest) {
 
 TEST(ReadLabFileTest, ParseTest) {
 	//This test is based on a test case from the original MRMC.
-   mrmc::dtmc::labeling* labeling;
+	mrmc::dtmc::labeling* labeling = NULL;
 
 	//Parsing the file
 	ASSERT_NO_THROW(labeling = mrmc::parser::read_lab_file(12, MRMC_CPP_TESTS_BASE_PATH "/parser/lab_files/pctl_general_input_01.lab"));
@@ -28,6 +28,7 @@ TEST(ReadLabFileTest, ParseTest) {
 
 	char phi[] = "phi", psi[] = "psi", smth[] = "smth";
 
+	if (labeling != NULL) {
 		ASSERT_TRUE(labeling->containsProposition(phi));
 		ASSERT_TRUE(labeling->containsProposition(psi));
 		ASSERT_TRUE(labeling->containsProposition(smth));
@@ -76,6 +77,7 @@ TEST(ReadLabFileTest, ParseTest) {
 		//Deleting the labeling
 		delete labeling;
 	}
+}
 
 TEST(ReadLabFileTest, WrongHeaderTest1) {
    ASSERT_THROW(mrmc::parser::read_lab_file(3, MRMC_CPP_TESTS_BASE_PATH "/parser/lab_files/wrong_format_header1.lab"), mrmc::exceptions::wrong_file_format);

test/parser/read_tra_file_test.cpp

@@ -23,9 +23,10 @@ TEST(ReadTraFileTest, NonExistingFileTest) {
  */
 TEST(ReadTraFileTest, ParseFileTest1) {
 	pantheios::log_INFORMATIONAL("Started ParseFileTest1");
-   mrmc::sparse::StaticSparseMatrix<double> *result;
+	mrmc::sparse::StaticSparseMatrix<double> *result = NULL;
 	ASSERT_NO_THROW(result = mrmc::parser::read_tra_file(MRMC_CPP_TESTS_BASE_PATH "/parser/tra_files/csl_general_input_01.tra"));
 
+	if (result != NULL) {
 		double val = 0;
 		ASSERT_TRUE(result->getValue(1,1,&val));
 		ASSERT_EQ(val,0.080645161290322580645161290322581);
@@ -62,6 +63,7 @@ TEST(ReadTraFileTest, ParseFileTest1) {
 		ASSERT_EQ(val,0);
 		delete result;
 	}
+}
 
 TEST(ReadTraFileTest, WrongFormatTestHeader1) {
    pantheios::log_INFORMATIONAL("Started WrongFormatTestHeader1");