Fix segfault in filterpolicy close (#60)

* Fix seq fault caused by double free. Now using API correctly.

* Add tests to cover autoClose.

rocksdb/columnfamily/cfopts.nim

@@ -11,12 +11,13 @@
 
 import ../lib/librocksdb, ../internal/utils, ../options/tableopts
 
+export tableopts
+
 type
   SlicetransformPtr* = ptr rocksdb_slicetransform_t
 
   SlicetransformRef* = ref object
     cPtr: SlicetransformPtr
-    autoClose*: bool # if true then close will be called when it's parent is closed
 
   ColFamilyOptionsPtr* = ptr rocksdb_options_t
 
@@ -25,7 +26,6 @@ type
     # type - CF options are a subset of rocksdb_options_t - when in doubt, check:
     # https://github.com/facebook/rocksdb/blob/b8c9a2576af6a1d0ffcfbb517dfcb7e7037bd460/include/rocksdb/options.h#L66
     cPtr: ColFamilyOptionsPtr
-    sliceTransform: SlicetransformRef
     tableOpts: TableOptionsRef
     autoClose*: bool # if true then close will be called when the database is closed
 
@@ -40,11 +40,8 @@ type
     xpressCompression = rocksdb_xpress_compression
     zstdCompression = rocksdb_zstd_compression
 
-proc createFixedPrefix*(value: int, autoClose = false): SlicetransformRef =
-  SlicetransformRef(
-    cPtr: rocksdb_slicetransform_create_fixed_prefix(value.csize_t),
-    autoClose: autoClose,
-  )
+proc createFixedPrefix*(value: int): SlicetransformRef =
+  SlicetransformRef(cPtr: rocksdb_slicetransform_create_fixed_prefix(value.csize_t))
 
 proc isClosed*(s: SlicetransformRef): bool {.inline.} =
   s.cPtr.isNil()
@@ -58,7 +55,7 @@ proc close*(s: SlicetransformRef) =
     rocksdb_slicetransform_destroy(s.cPtr)
     s.cPtr = nil
 
-proc newColFamilyOptions*(autoClose = false): ColFamilyOptionsRef =
+proc createColFamilyOptions*(autoClose = false): ColFamilyOptionsRef =
   ColFamilyOptionsRef(cPtr: rocksdb_options_create(), autoClose: autoClose)
 
 proc isClosed*(cfOpts: ColFamilyOptionsRef): bool {.inline.} =
@@ -73,7 +70,6 @@ proc close*(cfOpts: ColFamilyOptionsRef) =
     rocksdb_options_destroy(cfOpts.cPtr)
     cfOpts.cPtr = nil
 
-    autoCloseNonNil(cfOpts.sliceTransform)
     autoCloseNonNil(cfOpts.tableOpts)
 
 template opt(nname, ntyp, ctyp: untyped) =
@@ -126,7 +122,7 @@ opt blobCompactionReadaheadSize, int, uint64
 opt blobFileStartingLevel, int, cint
 
 proc defaultColFamilyOptions*(autoClose = false): ColFamilyOptionsRef =
-  newColFamilyOptions(autoClose)
+  createColFamilyOptions(autoClose)
 
 # proc setFixedPrefixExtractor*(dbOpts: ColFamilyOptionsRef, length: int) =
 #   doAssert not dbOpts.isClosed()
@@ -135,11 +131,14 @@ proc defaultColFamilyOptions*(autoClose = false): ColFamilyOptionsRef =
 
 proc `setPrefixExtractor`*(cfOpts: ColFamilyOptionsRef, value: SlicetransformRef) =
   doAssert not cfOpts.isClosed()
-  doAssert cfOpts.sliceTransform.isNil()
-    # don't allow overwriting an existing sliceTransform which could leak memory
 
+  # Destroys the existing slice transform if there is one attached to the column
+  # family options and takes ownership of the passed slice transform. After this call,
+  # the ColFamilyOptionsRef 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.
   rocksdb_options_set_prefix_extractor(cfOpts.cPtr, value.cPtr)
-  cfOpts.sliceTransform = value
+  value.cPtr = nil
 
 proc `blockBasedTableFactory=`*(
     cfOpts: ColFamilyOptionsRef, tableOpts: TableOptionsRef

rocksdb/options/backupopts.nim

@@ -18,7 +18,7 @@ type
     cPtr: BackupEngineOptionsPtr
     autoClose*: bool # if true then close will be called when the backup engine is closed
 
-proc newBackupEngineOptions*(autoClose = false): BackupEngineOptionsRef =
+proc createBackupEngineOptions*(autoClose = false): BackupEngineOptionsRef =
   BackupEngineOptionsRef(cPtr: rocksdb_options_create(), autoClose: autoClose)
 
 proc isClosed*(engineOpts: BackupEngineOptionsRef): bool {.inline.} =
@@ -31,7 +31,7 @@ proc cPtr*(engineOpts: BackupEngineOptionsRef): BackupEngineOptionsPtr =
 # TODO: Add setters and getters for backup options properties.
 
 proc defaultBackupEngineOptions*(autoClose = false): BackupEngineOptionsRef {.inline.} =
-  let opts = newBackupEngineOptions(autoClose)
+  let opts = createBackupEngineOptions(autoClose)
 
   # TODO: set defaults here
 

rocksdb/options/cache.nim

@@ -4,12 +4,19 @@ type
   CachePtr* = ptr rocksdb_cache_t
 
   CacheRef* = ref object
-    cPtr*: CachePtr
+    cPtr: CachePtr
     autoClose*: bool # if true then close will be called when it's parent is closed
 
 proc cacheCreateLRU*(size: int, autoClose = false): CacheRef =
   CacheRef(cPtr: rocksdb_cache_create_lru(size.csize_t), autoClose: autoClose)
 
+proc isClosed*(cache: CacheRef): bool =
+  isNil(cache.cPtr)
+
+proc cPtr*(cache: CacheRef): CachePtr =
+  doAssert not cache.isClosed()
+  cache.cPtr
+
 proc close*(cache: CacheRef) =
   if cache.cPtr != nil:
     rocksdb_cache_destroy(cache.cPtr)

rocksdb/options/dbopts.nim

@@ -21,7 +21,7 @@ type
     cache: CacheRef
     autoClose*: bool # if true then close will be called when the database is closed
 
-proc newDbOptions*(autoClose = false): DbOptionsRef =
+proc createDbOptions*(autoClose = false): DbOptionsRef =
   DbOptionsRef(cPtr: rocksdb_options_create(), autoClose: autoClose)
 
 proc isClosed*(dbOpts: DbOptionsRef): bool {.inline.} =
@@ -102,7 +102,7 @@ proc `rowCache=`*(dbOpts: DbOptionsRef, cache: CacheRef) =
   dbOpts.cache = cache
 
 proc defaultDbOptions*(autoClose = false): DbOptionsRef =
-  let opts: DbOptionsRef = newDbOptions(autoClose)
+  let opts: DbOptionsRef = createDbOptions(autoClose)
 
   # Optimize RocksDB. This is the easiest way to get RocksDB to perform well:
   opts.increaseParallelism(countProcessors())

rocksdb/options/readopts.nim

@@ -18,7 +18,7 @@ type
     cPtr: ReadOptionsPtr
     autoClose*: bool # if true then close will be called when the database is closed
 
-proc newReadOptions*(autoClose = false): ReadOptionsRef =
+proc createReadOptions*(autoClose = false): ReadOptionsRef =
   ReadOptionsRef(cPtr: rocksdb_readoptions_create(), autoClose: autoClose)
 
 proc isClosed*(readOpts: ReadOptionsRef): bool {.inline.} =
@@ -31,7 +31,7 @@ proc cPtr*(readOpts: ReadOptionsRef): ReadOptionsPtr =
 # TODO: Add setters and getters for read options properties.
 
 proc defaultReadOptions*(autoClose = false): ReadOptionsRef {.inline.} =
-  newReadOptions(autoClose)
+  createReadOptions(autoClose)
   # TODO: set prefered defaults
 
 proc close*(readOpts: ReadOptionsRef) =

rocksdb/options/tableopts.nim

@@ -1,20 +1,20 @@
 import ../lib/librocksdb, ../internal/utils, ./cache
 
+export cache
+
 type
   # TODO might eventually wrap this
   TableOptionsPtr* = ptr rocksdb_block_based_table_options_t
 
   TableOptionsRef* = ref object
-    cPtr*: TableOptionsPtr
+    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
+    cPtr: FilterPolicyPtr
 
   IndexType* {.pure.} = enum
     binarySearch = rocksdb_block_based_table_index_type_binary_search
@@ -27,21 +27,22 @@ 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 =
   isNil(policy.cPtr)
 
+proc cPtr*(policy: FilterPolicyRef): FilterPolicyPtr =
+  doAssert not policy.isClosed()
+  policy.cPtr
+
 proc close*(policy: FilterPolicyRef) =
   if not isClosed(policy):
     rocksdb_filterpolicy_destroy(policy.cPtr)
@@ -53,13 +54,9 @@ proc createTableOptions*(autoClose = false): TableOptionsRef =
 proc isClosed*(opts: TableOptionsRef): bool =
   isNil(opts.cPtr)
 
-proc close*(opts: TableOptionsRef) =
-  if not isClosed(opts):
-    rocksdb_block_based_options_destroy(opts.cPtr)
-    opts.cPtr = nil
-
-    autoCloseNonNil(opts.cache)
-    autoCloseNonNil(opts.filterPolicy)
+proc cPtr*(opts: TableOptionsRef): TableOptionsPtr =
+  doAssert not opts.isClosed()
+  opts.cPtr
 
 template opt(nname, ntyp, ctyp: untyped) =
   proc `nname=`*(opts: TableOptionsRef, value: ntyp) =
@@ -95,11 +92,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
@@ -109,3 +110,10 @@ proc defaultTableOptions*(autoClose = false): TableOptionsRef =
   opts.pinL0FilterAndIndexBlocksInCache = true
 
   opts
+
+proc close*(opts: TableOptionsRef) =
+  if not isClosed(opts):
+    rocksdb_block_based_options_destroy(opts.cPtr)
+    opts.cPtr = nil
+
+    autoCloseNonNil(opts.cache)

rocksdb/options/writeopts.nim

@@ -18,7 +18,7 @@ type
     cPtr: WriteOptionsPtr
     autoClose*: bool # if true then close will be called when the database is closed
 
-proc newWriteOptions*(autoClose = false): WriteOptionsRef =
+proc createWriteOptions*(autoClose = false): WriteOptionsRef =
   WriteOptionsRef(cPtr: rocksdb_writeoptions_create(), autoClose: autoClose)
 
 proc isClosed*(writeOpts: WriteOptionsRef): bool {.inline.} =
@@ -31,7 +31,7 @@ proc cPtr*(writeOpts: WriteOptionsRef): WriteOptionsPtr =
 # TODO: Add setters and getters for write options properties.
 
 proc defaultWriteOptions*(autoClose = false): WriteOptionsRef {.inline.} =
-  newWriteOptions(autoClose)
+  createWriteOptions(autoClose)
   # TODO: set prefered defaults
 
 proc close*(writeOpts: WriteOptionsRef) =

rocksdb/rocksdb.nim

@@ -363,7 +363,7 @@ proc openWriteBatch*(
   ## Opens a `WriteBatchRef` which defaults to using the specified column family.
   doAssert not db.isClosed()
 
-  newWriteBatch(cfHandle)
+  createWriteBatch(cfHandle)
 
 proc write*(db: RocksDbReadWriteRef, updates: WriteBatchRef): RocksDBResult[void] =
   ## Apply the updates in the `WriteBatchRef` to the database.
@@ -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)

rocksdb/transactions/txdbopts.nim

@@ -18,7 +18,7 @@ type
     cPtr: TransactionDbOptionsPtr
     autoClose*: bool # if true then close will be called when the database is closed
 
-proc newTransactionDbOptions*(autoClose = false): TransactionDbOptionsRef =
+proc createTransactionDbOptions*(autoClose = false): TransactionDbOptionsRef =
   TransactionDbOptionsRef(
     cPtr: rocksdb_transactiondb_options_create(), autoClose: autoClose
   )
@@ -35,7 +35,7 @@ proc cPtr*(txDbOpts: TransactionDbOptionsRef): TransactionDbOptionsPtr =
 proc defaultTransactionDbOptions*(
     autoClose = false
 ): TransactionDbOptionsRef {.inline.} =
-  newTransactionDbOptions(autoClose)
+  createTransactionDbOptions(autoClose)
   # TODO: set prefered defaults
 
 proc close*(txDbOpts: TransactionDbOptionsRef) =

rocksdb/transactions/txopts.nim

@@ -18,7 +18,7 @@ type
     cPtr: TransactionOptionsPtr
     autoClose*: bool # if true then close will be called when the transaction is closed
 
-proc newTransactionOptions*(autoClose = false): TransactionOptionsRef =
+proc createTransactionOptions*(autoClose = false): TransactionOptionsRef =
   TransactionOptionsRef(
     cPtr: rocksdb_transaction_options_create(), autoClose: autoClose
   )
@@ -33,7 +33,7 @@ proc cPtr*(txOpts: TransactionOptionsRef): TransactionOptionsPtr =
 # TODO: Add setters and getters for backup options properties.
 
 proc defaultTransactionOptions*(autoClose = false): TransactionOptionsRef {.inline.} =
-  newTransactionOptions(autoClose)
+  createTransactionOptions(autoClose)
   # TODO: set prefered defaults
 
 proc close*(txOpts: TransactionOptionsRef) =

rocksdb/writebatch.nim

@@ -22,7 +22,7 @@ type
     cPtr: WriteBatchPtr
     defaultCfHandle: ColFamilyHandleRef
 
-proc newWriteBatch*(defaultCfHandle: ColFamilyHandleRef): WriteBatchRef =
+proc createWriteBatch*(defaultCfHandle: ColFamilyHandleRef): WriteBatchRef =
   WriteBatchRef(cPtr: rocksdb_writebatch_create(), defaultCfHandle: defaultCfHandle)
 
 proc isClosed*(batch: WriteBatchRef): bool {.inline.} =

tests/columnfamily/test_cfhandle.nim

@@ -44,7 +44,7 @@ suite "ColFamilyHandleRef Tests":
     removeDir($dbPath)
 
   test "Test newColFamilyHandle":
-    var cfHandle = newColFamilyHandle(cfHandlePtr)
+    let cfHandle = newColFamilyHandle(cfHandlePtr)
 
     check:
       not cfHandle.cPtr.isNil()
@@ -53,7 +53,7 @@ suite "ColFamilyHandleRef Tests":
     cfHandle.close()
 
   test "Test close":
-    var cfHandle = newColFamilyHandle(cfHandlePtr)
+    let cfHandle = newColFamilyHandle(cfHandlePtr)
 
     check not cfHandle.isClosed()
     cfHandle.close()