SRF functions need to "fetch" (call FromDatum) each argument while …
#784
Conversation
…operating within the function's "multi-call-memory-context". This ensures any allocation `FromDatum` might do lives as along as the entire SRF calling process does. This is the proper fix for PR #779, which pointed out that it wasn't possible for SRFs to return data borrowed from their input.
There was a problem hiding this comment.
I tested my code against this PR and it fixed the use-after-free I was getting. (the tests now pass before and after reverting timescale/timescaledb-toolkit@fe8b386).
|
That's fantastic, @Smittyvb. Thanks for checking so quickly. We'll get this reviewed and merged ASAP. I'm hoping we can get another release out this week... |
| // function arguments need to be "fetched" while in the function call's | ||
| // multi-call-memory-context to ensure that any detoasted datums will | ||
| // live long enough for the SRF to use them over each call |
There was a problem hiding this comment.
This is what needs to also be documented in a place outside the context of this function's comments. Especially so because this is something that will always need to remain true: we always must satisfy this sort of lifetime-binding condition, and it doesn't matter whether the code is burned and rewritten a million times: it remains a constraint. It should be easier to reference this sort of information than reading thousands of lines of implementations line by line, because it determines how fast someone can begin to understand the concepts required to write correct code for implementing PGX.
| "name" => { | ||
| let mut out: NameMacro = mac.parse_body().expect("name!() in wrong format"); | ||
| staticize_lifetimes(&mut out.used_ty.resolved_ty); | ||
|
|
||
| // rewrite the name!() macro so that it has a static lifetime, if any | ||
| let ident = syn::parse_str::<Ident>(&out.ident).unwrap(); | ||
| let ty = out.used_ty.resolved_ty; | ||
| type_macro.mac = syn::parse_quote! {name!(#ident, #ty)}; |
There was a problem hiding this comment.
#[pg_extern]
fn split_table_with_borrow<'a>(
input: &'a str,
pattern: &'a str,
) -> TableIterator<'a, (name!(i, i32), name!(s, &'a str))>So in this case, the return lifetimes we are expecting will be
) -> TableIterator<'static, (name!(i, i32), name!(s, &'static str))>?
There was a problem hiding this comment.
Hm. I think this can be done in a better way in the future, but I can't find the solution immediately and we're staticizing other lifetimes, so I think this is acceptable for the current moment.
597: Revert now unneeded pgx workaround r=Smittyvb a=Smittyvb We don't need copy everything into memory owned by Rust anymore, since pgx 0.5.4 fixed the bug that made doing that necessary (pgcentralfoundation/pgrx#784). This reverts commit fe8b386. I didn't add anything to the changelog since this an internal change that doesn't affect end users. Co-authored-by: Smitty <smitty@timescale.com>
…operating within the function's "multi-call-memory-context". This ensures any allocation
FromDatummight do lives as along as the entire SRF calling process does.This is the proper fix for PR #779, which pointed out that it wasn't possible for SRFs to return data borrowed from their input. Tests are included.
@Smittyvb: This PR should resolve the problems that #779 was trying to explain away with documentation. Prior to this PR, pgx was potentially detoasting argument datums in the wrong memory context, causing the UAF you were seeing. I'd love it if you could rejigger your code against this PR and make sure it now works as you'd expect. I updated the
stringsexample to show that a SRF can borrow from its input.