Conversation
|
Claude finished @khustup2's task in 1m 4s —— View job Reviewing the PR changes... Tasks
Review SummaryFound 6 significant issues requiring attention: Security/Correctness:
Resource/Concurrency:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| initStringInfo(&check_sql); | ||
| appendStringInfo(&check_sql, | ||
| "SELECT 1 FROM pg_database WHERE datname = '%s'", | ||
| db.db_name.c_str()); |
There was a problem hiding this comment.
SQL injection vulnerability: database name concatenated into query without sanitization. Use parameterized query instead.
| db.db_name.c_str()); | |
| appendStringInfo(&check_sql, | |
| "SELECT 1 FROM pg_database WHERE datname = $1"); |
Then use SPI_execute_with_args with values array.
| appendStringInfo(&create_sql, " OWNER %s", quote_identifier(db.owner.c_str())); | ||
| } | ||
| if (!db.encoding.empty()) { | ||
| appendStringInfo(&create_sql, " ENCODING '%s'", db.encoding.c_str()); |
There was a problem hiding this comment.
SQL injection risk: db.encoding, db.lc_collate, db.lc_ctype from external catalog inserted directly. These should be validated or quoted properly as they could contain SQL metacharacters.
| continue; | ||
| } | ||
|
|
||
| PGresult* res = PQexec(conn, check_sql.data); |
There was a problem hiding this comment.
Resource leak: PGresult freed before potential early return at line 264. Move pfree(check_sql.data) after PQfinish to avoid leaking StringInfo memory on connection failure.
| } | ||
|
|
||
| LWLockRelease(data_->lock); | ||
| return ok; |
There was a problem hiding this comment.
Silent queue overflow: when queue is full (count >= MAX_PENDING), returns false but caller at extension_init.cpp:719 ignores return value. Database install will be silently skipped.
| // Queue the database for async extension install by the sync worker. | ||
| // The inline PQconnectdb approach fails on PG15+ because CREATE DATABASE | ||
| // is WAL-logged/transactional and the pg_database row isn't committed yet. | ||
| pg::pending_install_queue::enqueue(dbstmt->dbname); |
There was a problem hiding this comment.
Ignoring return value from enqueue. If queue is full, extension install will be silently skipped with no warning to user.
| pg::pending_install_queue::enqueue(dbstmt->dbname); | |
| if (!pg::pending_install_queue::enqueue(dbstmt->dbname)) { | |
| elog(WARNING, "pg_deeplake: failed to queue database '%s' for extension install (queue full)", dbstmt->dbname); | |
| } |
| } | ||
|
|
||
| auto it = latest.find(meta.db_name); | ||
| if (it == latest.end() || it->second.updated_at <= meta.updated_at) { |
There was a problem hiding this comment.
Race condition: updated_at <= meta.updated_at allows ties. Two concurrent upserts with same timestamp result in non-deterministic latest selection (map insertion order dependent).
|
1 participant
🚀 🚀 Pull Request
Impact
Description
Things to be aware of
Things to worry about
Additional Context