Merge pull request #10712 from Fermions-ASI/fix/10082

odb: validate 3dblox input file paths before creating DB objects (#10082)

Author: maliberty

src/odb/src/3dblox/baseParser.cpp

@@ -11,6 +11,7 @@
 #include <vector>
 
 #include "objects.h"
+#include "utl/CFileUtils.h"
 #include "utl/Logger.h"
 #include "yaml-cpp/yaml.h"
 namespace odb {
@@ -178,6 +179,11 @@ void BaseParser::logError(const std::string& message)
       utl::ODB, 521, "Parser Error in {}: {}", current_file_path_, message);
 }
 
+std::ifstream BaseParser::openInputFile()
+{
+  return utl::OpenInputStream(current_file_path_, logger_);
+}
+
 std::string BaseParser::trim(const std::string& str)
 {
   size_t start = str.find_first_not_of(" \t\n\r");

src/odb/src/3dblox/baseParser.h

@@ -3,6 +3,7 @@
 
 #pragma once
 
+#include <fstream>
 #include <string>
 #include <vector>
 
@@ -37,6 +38,13 @@ class BaseParser
   std::string resolvePath(const std::string& path);
   void resolvePaths(const std::string& path, std::vector<std::string>& paths);
 
+  // Opens an input file for reading after validating that the path exists and
+  // refers to a regular, readable file. On any failure this reports a clean
+  // logger error (which is non-returning) BEFORE the caller creates any DB
+  // objects, preventing downstream crashes on invalid/nonexistent paths.
+  // current_file_path_ must be set to the file path before calling.
+  std::ifstream openInputFile();
+
   // Utility methods
   void logError(const std::string& message);
   std::string trim(const std::string& str);

src/odb/src/3dblox/bmapParser.cpp

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "objects.h"
+#include "utl/CFileUtils.h"
 #include "utl/Logger.h"
 namespace odb {
 
@@ -20,11 +21,7 @@ BmapParser::BmapParser(utl::Logger* logger) : logger_(logger)
 BumpMapData BmapParser::parseFile(const std::string& filename)
 {
   current_file_path_ = filename;
-  std::ifstream file(filename);
-  if (!file.is_open()) {
-    logger_->error(
-        utl::ODB, 535, "Bump Map Parser Error: Cannot open file: {}", filename);
-  }
+  std::ifstream file = utl::OpenInputStream(filename, logger_);
 
   std::stringstream buffer;
   buffer << file.rdbuf();

src/odb/src/3dblox/dbvParser.cpp

@@ -25,10 +25,7 @@ DbvParser::DbvParser(utl::Logger* logger) : BaseParser(logger)
 DbvData DbvParser::parseFile(const std::string& filename)
 {
   current_file_path_ = filename;
-  std::ifstream file(filename);
-  if (!file.is_open()) {
-    logError("Cannot open file");
-  }
+  std::ifstream file = openInputFile();
 
   std::stringstream buffer;
   buffer << file.rdbuf();

src/odb/src/3dblox/dbxParser.cpp

@@ -24,10 +24,7 @@ DbxParser::DbxParser(utl::Logger* logger) : BaseParser(logger)
 DbxData DbxParser::parseFile(const std::string& filename)
 {
   current_file_path_ = filename;
-  std::ifstream file(filename);
-  if (!file.is_open()) {
-    logError("Cannot open file");
-  }
+  std::ifstream file = openInputFile();
 
   std::stringstream buffer;
   buffer << file.rdbuf();

src/odb/test/BUILD

@@ -220,6 +220,7 @@ COMPULSORY_TESTS = [
 PASSFAIL_TESTS = [
     # pass-fail
     "read_3dbv_fail",
+    "read_3dbx_fail",
     "read_def_virtual_invalid",
     "test_block",
     "test_bterm",

src/odb/test/CMakeLists.txt

@@ -74,6 +74,7 @@ or_integration_tests(
   PASSFAIL_TESTS
     cpp_tests
     read_3dbv_fail
+    read_3dbx_fail
     read_def_virtual_invalid
     test_block
     test_bterm

src/odb/test/read_3dbx_fail.tcl

@@ -0,0 +1,38 @@
+# Regression for issue #10082: read_3dbx must report a clean error (not crash)
+# on an invalid / nonexistent path, instead of proceeding to create DB objects
+# with an uninitialized state and segfaulting.
+source "helpers.tcl"
+
+# Test 1: nonexistent file passed to read_3dbx.
+# Caught by the read_3dbx Tcl wrapper before reaching the C++ reader.
+set status [catch { read_3dbx "data/this_file_does_not_exist.3dbx" } msg]
+if { $status != 1 } {
+  puts "fail: read_3dbx accepted a nonexistent path"
+  exit 1
+}
+if { ![regexp "ORD-0072" $msg] } {
+  puts "fail: unexpected error for nonexistent path: $msg"
+  exit 1
+}
+
+# Test 2: invalid path that exists but is not a regular file (a directory).
+# This bypasses the Tcl wrapper's "file exists" check and exercises the C++
+# 3dblox reader's file-validation guard directly. Before the fix the reader's
+# std::ifstream::is_open() succeeded on a directory (on Linux), so parsing
+# proceeded on empty content and could create DB objects with uninitialized
+# state -> segfault. Call the C++ command directly to reach that path.
+set dir_path [make_result_file "read_3dbx_fail_dir.3dbx"]
+file delete -force $dir_path
+file mkdir $dir_path
+set status [catch { ord::read_3dbx_cmd $dir_path } msg]
+file delete -force $dir_path
+if { $status != 1 } {
+  puts "fail: read_3dbx_cmd accepted a directory path"
+  exit 1
+}
+if { ![regexp "UTL-0015" $msg] } {
+  puts "fail: unexpected error for directory path: $msg"
+  exit 1
+}
+
+puts "pass"

src/utl/include/utl/CFileUtils.h

@@ -5,6 +5,7 @@
 
 #include <cstdint>
 #include <cstdio>
+#include <fstream>
 #include <span>
 #include <string>
 
@@ -35,4 +36,11 @@ std::string GetContents(FILE* file, Logger* logger);
 // of "data" has been written successfully.
 void WriteAll(FILE* file, std::span<const uint8_t> data, Logger* logger);
 
+// Opens "filename" as a text input stream after validating that it refers to an
+// existing regular file. std::ifstream::is_open() alone is insufficient: on
+// Linux it succeeds for directories (and other non-regular files), so the
+// caller would silently read empty content. On failure logger->error is used to
+// throw.
+std::ifstream OpenInputStream(const std::string& filename, Logger* logger);
+
 }  // namespace utl

src/utl/src/CFileUtils.cpp

@@ -6,6 +6,8 @@
 #include <cstdint>
 #include <cstdio>
 #include <cstring>
+#include <filesystem>
+#include <fstream>
 #include <span>
 #include <string>
 
@@ -76,4 +78,20 @@ void WriteAll(FILE* file, std::span<const uint8_t> data, Logger* logger)
   }
 }
 
+std::ifstream OpenInputStream(const std::string& filename, Logger* logger)
+{
+  // is_regular_file() returns false for a nonexistent path, a directory, or any
+  // other non-regular file, so a single check rejects all the cases where a
+  // plain ifstream would otherwise open (or appear to open) and read nothing.
+  std::error_code ec;
+  if (!std::filesystem::is_regular_file(filename, ec)) {
+    logger->error(UTL, 15, "{} is not a readable file", filename);
+  }
+  std::ifstream stream(filename);
+  if (!stream.is_open()) {
+    logger->error(UTL, 16, "failed to open file {}", filename);
+  }
+  return stream;
+}
+
 }  // namespace utl