Skip to content

Commit d2d84fa

Browse files
authored
Fix memory leak in Value Display implementation (#562)
The original code used `CString::from_raw()`, which takes ownership of the C string but never frees the underlying memory allocated by DuckDB. Worse, assuming ownership of memory that belonged to DuckDB's memory allocator can lead to undefined behavior. Fixes #197
2 parents 4a4203f + 4e67d67 commit d2d84fa

File tree

1 file changed

+24
-4
lines changed

1 file changed

+24
-4
lines changed

crates/duckdb/src/vtab/value.rs

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
use crate::ffi::{duckdb_destroy_value, duckdb_get_int64, duckdb_get_varchar, duckdb_value};
2-
use std::{ffi::CString, fmt};
1+
use crate::ffi::{duckdb_destroy_value, duckdb_free, duckdb_get_int64, duckdb_get_varchar, duckdb_value};
2+
use std::{ffi::CStr, fmt, os::raw::c_void};
33

44
/// The Value object holds a single arbitrary value of any type that can be
55
/// stored in the database.
@@ -34,7 +34,27 @@ impl Value {
3434

3535
impl fmt::Display for Value {
3636
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
37-
let c_string = unsafe { CString::from_raw(duckdb_get_varchar(self.ptr)) };
38-
write!(f, "{}", c_string.to_str().unwrap())
37+
unsafe {
38+
let varchar = duckdb_get_varchar(self.ptr);
39+
let c_str = CStr::from_ptr(varchar);
40+
let res = write!(f, "{}", c_str.to_string_lossy());
41+
duckdb_free(varchar as *mut c_void);
42+
res
43+
}
44+
}
45+
}
46+
47+
#[cfg(test)]
48+
mod tests {
49+
use super::*;
50+
use crate::ffi::duckdb_create_varchar;
51+
use std::ffi::CString;
52+
53+
#[test]
54+
fn test_value_to_string() {
55+
let c_str = CString::new("some value").unwrap();
56+
let duckdb_val = unsafe { duckdb_create_varchar(c_str.as_ptr()) };
57+
let val = Value::from(duckdb_val);
58+
assert_eq!(val.to_string(), "some value");
3959
}
4060
}

0 commit comments

Comments
 (0)