odb: UnfoldedModel as a persistent object in dbDatabase#9639
odb: UnfoldedModel as a persistent object in dbDatabase#9639osamahammad21 wants to merge 4 commits intoThe-OpenROAD-Project:masterfrom
Conversation
Signed-off-by: osamahammad21 <osama21@aucegypt.edu>
|
clang-tidy review says "All clean, LGTM! 👍" |
![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 refactors the creation of UnfoldedModel to be a persistent, lazily-initialized object within dbDatabase, which is a good change for performance. However, the current implementation introduces a high-severity Use-After-Free vulnerability due to the lack of an invalidation mechanism when the underlying database is modified. This also leads to cache staleness, potentially returning incorrect data. Furthermore, the lazy initialization is not thread-safe, which can cause race conditions, and a potential null pointer dereference was identified in the Checker::check method when operating on an empty database. These issues need to be addressed to ensure correctness, stability, and security.
src/odb/src/db/dbDatabase.cpp
Outdated
| UnfoldedModel* dbDatabase::getUnfoldedModel() | ||
| { | ||
| _dbDatabase* db = (_dbDatabase*) this; | ||
| if (db->unfolded_model_ == nullptr) { | ||
| db->unfolded_model_ = new UnfoldedModel(db->logger_, getChip()); | ||
| } | ||
| return db->unfolded_model_; | ||
| } |
There was a problem hiding this comment.
This section of code is susceptible to a Use-After-Free vulnerability. The UnfoldedModel is a persistent, lazily-initialized object, but it lacks an invalidation mechanism. When the underlying database (e.g., dbChip or its components) is modified, the UnfoldedModel can retain dangling pointers to deleted objects, leading to crashes or unpredictable behavior upon access. Additionally, this results in stale data being returned. To remediate this, an invalidation mechanism must be implemented. The unfolded_model_ should be deleted and set to nullptr whenever the database structure is modified (e.g., via observers, explicitly clearing it in modification methods, or by adding an invalidateUnfoldedModel() method to dbDatabase and calling it after relevant structural changes).
Signed-off-by: osamahammad21 <osama21@aucegypt.edu>
Signed-off-by: osamahammad21 <osama21@aucegypt.edu>
Signed-off-by: osamahammad21 <osama21@aucegypt.edu>
|
clang-tidy review says "All clean, LGTM! 👍" |
| void dbDatabase::constructUnfoldedModel() | ||
| { | ||
| _dbDatabase* db = (_dbDatabase*) this; | ||
| absl::MutexLock lock(unfolded_model_mutex); |
There was a problem hiding this comment.
In general odb doesn't support MT access. Do we need it here?
If so we need to guard against the model already being created by the previous locker. Also getUnfoldedModel will need a lock.
| return (dbChip*) db->chip_tbl_->getPtr(db->chip_); | ||
| } | ||
|
|
||
| void dbDatabase::constructUnfoldedModel() |
There was a problem hiding this comment.
Should we have auto creation in get rather than explicit construction? Do we want get to return a nullptr?
|
|
||
| utl::Logger* logger_; | ||
| std::set<dbDatabaseObserver*> observers_; | ||
| UnfoldedModel* unfolded_model_; |
There was a problem hiding this comment.
mention that it is non-persistent
2 participants
No description provided.