close index file fd only if index is not build via socket#331
Conversation
There was a problem hiding this comment.
PR Summary
This pull request addresses a critical issue in the LanternDB extension, fixing crashes related to improper file descriptor handling during index building.
- Modified
src/hnsw/build.cto close the index file descriptor only when building from an existing file, preventing crashes when using external sockets - Added assertion in
src/hnsw/build.cto ensure valid file descriptor when building from existing file - Updated version from 0.3.2 to 0.3.3 in
CMakeLists.txt, reflecting the bug fix - Added new SQL update file
sql/updates/0.3.2--0.3.3.sqlfor version migration (contents unavailable for review)
3 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings
| include(CheckSymbolExists) | ||
|
|
||
| set(LANTERN_VERSION 0.3.2) | ||
| set(LANTERN_VERSION 0.3.3) |
There was a problem hiding this comment.
info: updated version number from 0.3.2 to 0.3.3
| index_file_fd = open(buildstate->index_file_path, O_RDONLY); | ||
| assert(index_file_fd > 0); |
There was a problem hiding this comment.
info: Added assertion to ensure index_file_fd is valid when opened
| munmap_ret = munmap(result_buf, index_file_stat.st_size); | ||
| assert(munmap_ret == 0); | ||
| LDB_UNUSED(munmap_ret); | ||
| close(index_file_fd); |
There was a problem hiding this comment.
info: Moved close(index_file_fd) inside the conditional block where it's used
| LDB_UNUSED(munmap_ret); | ||
| close(index_file_fd); | ||
| } | ||
|
|
There was a problem hiding this comment.
info: Removed unconditional close(index_file_fd) to prevent undefined behavior
Benchmarks
|
35aad5b to
072283a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
We were calling
closesyscall onindex_file_fdvariable for all cases when building the index, but when building index with external socket no index file is used, soindex_file_fdis not allocated. This led to undefined behaviour and led to crashes in some systems with errors like:error: could not seek to end of file "base/5/2610": Bad file descriptororPANIC: could not write to log file 00000001000000000000004C at offset 13369344, length 8192: Bad file descriptor