Skip to content

Commit

Permalink
fix segfault converting a null pg_sys::Datum to a String (#1853)
Browse files Browse the repository at this point in the history
Looks like this was introduced in beb7901 and we didn't have test coverage around this specific case.
  • Loading branch information
eeeebbbbrrrr authored Sep 11, 2024
1 parent 5eadffb commit d13cd30
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 14 deletions.
13 changes: 13 additions & 0 deletions pgrx-tests/src/tests/from_into_datum_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,17 @@ mod tests {
assert_eq!(cstr, expected);
Ok(())
}

#[pg_test]
fn null_string_is_none() {
let val: pg_sys::Datum = pg_sys::Datum::null();
let is_null: bool = true;

unsafe {
if let Some(_) = String::from_datum(val, is_null) {
// <- this seg fault
panic!("converted a null Datum into a valid string")
}
}
}
}
32 changes: 18 additions & 14 deletions pgrx/src/datum/from.rs
Original file line number Diff line number Diff line change
Expand Up @@ -401,23 +401,27 @@ impl FromDatum for String {
#[inline]
unsafe fn from_polymorphic_datum(
datum: pg_sys::Datum,
_is_null: bool,
is_null: bool,
_typoid: pg_sys::Oid,
) -> Option<String> {
let varlena = pg_sys::pg_detoast_datum_packed(datum.cast_mut_ptr());
let converted_varlena = convert_varlena_to_str_memoized(varlena);
let ret_string = converted_varlena.to_owned();

// If the datum is EXTERNAL or COMPRESSED, then detoasting creates a pfree-able chunk
// that needs to be freed. We can free it because `to_owned` above creates a Rust copy
// of the string.
if varlena::varatt_is_1b_e(datum.cast_mut_ptr())
|| varlena::varatt_is_4b_c(datum.cast_mut_ptr())
{
pg_sys::pfree(varlena.cast());
}
if is_null || datum.is_null() {
return None;
} else {
let varlena = pg_sys::pg_detoast_datum_packed(datum.cast_mut_ptr());
let converted_varlena = convert_varlena_to_str_memoized(varlena);
let ret_string = converted_varlena.to_owned();

// If the datum is EXTERNAL or COMPRESSED, then detoasting creates a pfree-able chunk
// that needs to be freed. We can free it because `to_owned` above creates a Rust copy
// of the string.
if varlena::varatt_is_1b_e(datum.cast_mut_ptr())
|| varlena::varatt_is_4b_c(datum.cast_mut_ptr())
{
pg_sys::pfree(varlena.cast());
}

Some(ret_string)
Some(ret_string)
}
}
}

Expand Down

0 comments on commit d13cd30

Please sign in to comment.