odb: fix segfault on invalid path in read_3dbx#10119
odb: fix segfault on invalid path in read_3dbx#10119oksaumya wants to merge 6 commits intoThe-OpenROAD-Project:masterfrom
Conversation
Add explicit file existence checks in ThreeDBlox::readDbx and ThreeDBlox::readDbv before parsing. Previously, when a non-existent path was passed (directly or via a header include), execution could continue past the failed open, producing partially-initialized ChipletDef data that caused a segfault in dbBlock::create via _dbBlock::initialize -> dbModule::create -> _dbDatabase::getLogger(). Also guard dbBlock::create against a null tech pointer to prevent a secondary crash when a chip has no associated technology. Add regression test read_3dbx-fail covering: - main 3dbx file does not exist - main file exists but references a non-existent include Fixes The-OpenROAD-Project#10082 echo "Message file created" Signed-off-by: saumya <saumyakr2006@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces file existence checks for 3DBV and 3DBX files in the 3DBlox module to prevent crashes and provide better error reporting. It also includes a null check for the technology object in dbBlock::create and adds a regression test suite to verify error handling for missing files. A review comment points out a duplicate error ID used in the logging for missing 3DBX files, which should be updated to a unique value.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Prevents crashes when reading 3DBlox files by hardening file existence checks and guarding block creation when technology is absent, plus adds a regression test for the failure paths described in #10082.
Changes:
- Add
std::filesystem::existschecks inThreeDBlox::readDbx/readDbvbefore parsing. - Guard
dbBlock::createagainst a nullchip_->getTech()dereference. - Add
read_3dbx-failregression test and register it in CMake + Bazel.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/odb/src/3dblox/3dblox.cpp | Adds pre-parse file existence checks to avoid deep segfault paths. |
| src/odb/src/db/dbBlock.cpp | Avoids null dereference when tech is absent on certain chips. |
| src/odb/test/read_3dbx-fail.tcl | Adds regression test covering missing main file and missing include. |
| src/odb/test/read_3dbx-fail.ok | Golden output for the new regression test. |
| src/odb/test/data/fail.3dbx | New fixture referencing a missing include file. |
| src/odb/test/CMakeLists.txt | Registers the new test with CMake. |
| src/odb/test/BUILD | Registers the new test and test data with Bazel. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to OpenROAD! Thanks for opening your first PR.
Before we review:
- Contribution Guide: https://openroad.readthedocs.io/en/latest/contrib/contributing.html
- Build Instructions: https://openroad.readthedocs.io/en/latest/contrib/BuildWithCMake.html
Please ensure:
- CI passes
- Code is properly formatted
- Tests are included where applicable
A maintainer will review shortly!
- Use std::error_code overloads for std::filesystem::exists in readDbx and readDbv to handle permission errors or bad paths - Fix duplicate error ID: readDbx check changed from 542 to 556 (542 was already used for bump net bterm error at line 949) - Add else-if fallback in dbBlock::create to guard dbu_per_micron_ when chip has no associated technology - Strengthen read_3dbx-fail test to assert err_msg contains the expected substring not just any error Signed-off-by: saumya <saumyakr2006@gmail.com>
|
@oksaumya this check if the 3DBV file is missing (which we should also be doing), but the actual error I reported was due to the DEF file being missing (which I think there is a warning about that). |
The actual crash reported in The-OpenROAD-Project#10082 occurs when a chiplet's external DEF file does not exist: defin::createBlock does fopen, fails, emits a warning and returns false; readChip then destroys the chip and returns; but createChiplet continues using the now-dangling chip pointer, causing a segfault on chip->setWidth() etc. Add a std::filesystem::exists check (non-throwing error_code overload) in createChiplet before calling def_reader.readChip, so a proper error is raised before any chip is created or destroyed. Also extend the read_3dbx-fail regression test with a third case that exercises read_3dbv with a chiplet referencing a nonexistent DEF file. Signed-off-by: saumya <saumyakr2006@gmail.com>
|
@gadfort Thanks for the clarification.
The fix adds a std::filesystem::exists check (non-throwing error_code overload) in createChiplet before calling def_reader.readChip, so a proper error is raised before any chip object is created or destroyed. I've also added a third test case to the read_3dbx-fail regression test that exercises read_3dbv with a chiplet referencing a nonexistent DEF file, confirming a clean error instead of a crash. |
- Remove redundant filesystem::exists checks in readDbv/readDbx;
DbvParser::parseFile and DbxParser::parseFile already call logError
(-> logger_->error) when ifstream fails to open the file
- In dbBlock::create, use chip_->getDatabase()->getDbuPerMicron() as
the fallback dbu_per_micron when tech is null, instead of hardcoding 1
- Update regression test to match actual parser error message
("Cannot open file") for missing 3DBX/3DBV files
Signed-off-by: saumya <saumyakr2006@gmail.com>
|
@osamahammad21 fixed all suggestions |
|
@oksaumya Please address the workflow failures. |
- Simplify dbBlock::create to use chip_->getDb()->getDbuPerMicron() directly, replacing the if/else tech null check as suggested by maintainer - Add missing #include <system_error> for std::error_code per clang-tidy - Fix alphabetical ordering of data files in Bazel BUILD regression_resources Signed-off-by: saumya <saumyakr2006@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent crashes when reading 3DBlox inputs that reference missing files, and adds a regression test intended to validate the new failure behavior.
Changes:
- Add a filesystem existence check for chiplet DEF files before attempting to read them.
- Avoid dereferencing a potentially-null tech pointer when initializing a new
dbBlock’s DBU scale. - Add a new regression test (
read_3dbx-fail) and register it in both CMake and Bazel test lists.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/odb/src/3dblox/3dblox.cpp | Adds a DEF file existence check during chiplet creation. |
| src/odb/src/db/dbBlock.cpp | Sets dbu_per_micron_ from the database rather than through chip_->getTech(). |
| src/odb/test/read_3dbx-fail.tcl | New Tcl regression test covering missing main/include/DEF file cases. |
| src/odb/test/read_3dbx-fail.ok | Expected stdout for the new regression test. |
| src/odb/test/data/fail.3dbx | Test input referencing a missing include file. |
| src/odb/test/data/fail_def.3dbv | Test input referencing a missing DEF file. |
| src/odb/test/CMakeLists.txt | Registers the new read_3dbx-fail integration test. |
| src/odb/test/BUILD | Registers the new test and adds its data files to Bazel resources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Split DEF file check in createChiplet: ODB-0558 for filesystem access errors (e.g. permissions), ODB-0557 for file-not-found, so the error message is actionable in both cases - Update regression test assertions to match actual thrown error IDs: ORD-0072 for missing main file (Tcl proc catches before C++ layer), ODB-0521 for missing include (BaseParser::logError), ODB-0557 for missing DEF file (createChiplet check) Signed-off-by: saumya <saumyakr2006@gmail.com>
Fixes #10082
Summary
std::filesystem::existschecks inThreeDBlox::readDbxandThreeDBlox::readDbvbefore parsing, preventing execution from continuing with a missing file that could produce partially-initializedChipletDefdata and segfault deep indbBlock::create -> _dbBlock::initialize -> dbModule::create -> _dbDatabase::getLogger()dbBlock::createagainst a null tech pointer (chip_->getTech()can be null for non-DIE chips without an associated technology)read_3dbx-failcovering both failure modes: missing main file and missing include fileStack trace fixed
2# odb::_dbDatabase::getLogger() const
3# odb::dbModule::create(odb::dbBlock*, char const*)
4# odb::_dbBlock::initialize(odb::_dbChip*, odb::_dbBlock*, char const*, char)
5# odb::dbBlock::create(odb::dbChip*, char const*, char)
6# odb::ThreeDBlox::createChiplet(odb::ChipletDef const&)
7# odb::ThreeDBlox::readDbv(...)
8# odb::ThreeDBlox::readHeaderIncludes(...)
9# odb::ThreeDBlox::readDbx(...)
Test plan
read_3dbx-failtest passes: catches proper error for non-existent file instead of segfaultingread_3dbxandread_3dbvtests still pass (no regression)