Skip to content

Commit

Permalink
Fix seq fault caused by double free. Now using API correctly.
Browse files Browse the repository at this point in the history
  • Loading branch information
bhartnett committed Jul 1, 2024
1 parent caedba0 commit 89232b1
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 15 deletions.
20 changes: 9 additions & 11 deletions rocksdb/options/tableopts.nim
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,12 @@ type
TableOptionsRef* = ref object
cPtr*: TableOptionsPtr
cache: CacheRef
filterPolicy: FilterPolicyRef
autoClose*: bool # if true then close will be called when it's parent is closed

FilterPolicyPtr* = ptr rocksdb_filterpolicy_t

FilterPolicyRef* = ref object
cPtr*: FilterPolicyPtr
autoClose*: bool # if true then close will be called when it's parent is closed

IndexType* {.pure.} = enum
binarySearch = rocksdb_block_based_table_index_type_binary_search
Expand All @@ -27,16 +25,13 @@ type
rocksdb_block_based_table_data_block_index_type_binary_search_and_hash

proc createRibbon*(bitsPerKey: float, autoClose = false): FilterPolicyRef =
FilterPolicyRef(
cPtr: rocksdb_filterpolicy_create_ribbon(bitsPerKey), autoClose: autoClose
)
FilterPolicyRef(cPtr: rocksdb_filterpolicy_create_ribbon(bitsPerKey))

proc createRibbonHybrid*(
bitsPerKey: float, bloomBeforeLevel: int = 0, autoClose = false
): FilterPolicyRef =
FilterPolicyRef(
cPtr: rocksdb_filterpolicy_create_ribbon_hybrid(bitsPerKey, bloomBeforeLevel.cint),
autoClose: autoClose,
cPtr: rocksdb_filterpolicy_create_ribbon_hybrid(bitsPerKey, bloomBeforeLevel.cint)
)

proc isClosed*(policy: FilterPolicyRef): bool =
Expand All @@ -59,7 +54,6 @@ proc close*(opts: TableOptionsRef) =
opts.cPtr = nil

autoCloseNonNil(opts.cache)
autoCloseNonNil(opts.filterPolicy)

template opt(nname, ntyp, ctyp: untyped) =
proc `nname=`*(opts: TableOptionsRef, value: ntyp) =
Expand Down Expand Up @@ -95,11 +89,15 @@ proc `blockCache=`*(opts: TableOptionsRef, cache: CacheRef) =

proc `filterPolicy=`*(opts: TableOptionsRef, policy: FilterPolicyRef) =
doAssert not opts.isClosed()
doAssert opts.filterPolicy.isNil()
# don't allow overwriting an existing policy which could leak memory

# Destroys the existing policy if there is one attached to the table options
# and takes ownership of the passed in policy. After this call, the TableOptionsRef
# is responsible for cleaning up the policy when it is no longer needed
# so we set the filter policy to nil so that isClosed() will return true
# and prevent the filter policy from being double freed which was causing a seg fault.
# See here: https://github.com/facebook/rocksdb/blob/22fe23edc89e9842ed72b613de172cd80d3b00da/include/rocksdb/filter_policy.h#L152
rocksdb_block_based_options_set_filter_policy(opts.cPtr, policy.cPtr)
opts.filterPolicy = policy
policy.cPtr = nil

proc defaultTableOptions*(autoClose = false): TableOptionsRef =
# https://github.com/facebook/rocksdb/wiki/Setup-Options-and-Basic-Tuning#other-general-options
Expand Down
5 changes: 1 addition & 4 deletions rocksdb/rocksdb.nim
Original file line number Diff line number Diff line change
Expand Up @@ -417,10 +417,7 @@ proc close*(db: RocksDbRef) =
# opts should be closed after the database is closed
autoCloseNonNil(db.dbOpts)
autoCloseNonNil(db.readOpts)

for cfDesc in db.cfDescriptors:
if cfDesc.autoClose:
cfDesc.close()
autoCloseAll(db.cfDescriptors)

if db of RocksDbReadWriteRef:
let db = RocksDbReadWriteRef(db)
Expand Down

0 comments on commit 89232b1

Please sign in to comment.