Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

register_hook(&mut HOOKS) : mutable reference to mutable static #1749

Open
daamien opened this issue Jul 4, 2024 · 5 comments
Open

register_hook(&mut HOOKS) : mutable reference to mutable static #1749

daamien opened this issue Jul 4, 2024 · 5 comments

Comments

@daamien
Copy link
Contributor

daamien commented Jul 4, 2024

Not a actually bug, but a strange warning....

I registered my hooks with the following line

        static mut HOOKS: AnonHook = AnonHook { };
        pgrx::hooks::register_hook(&mut HOOKS)

which I copy-pasted blindly from the hook_tests here:

https://github.com/pgcentralfoundation/pgrx/blob/2c1d3887d16744e5230efe506d98a51836cf8228/pgrx-tests/src/tests/hooks_tests.rs#L126C1-L127C46

Cargo 1.77 issues the following warning

473 |     pgrx::hooks::register_hook(&mut HOOKS);
    |                                ^^^^^^^^^^ mutable reference to mutable static
    |
    = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447>
    = note: this will be a hard error in the 2024 edition
    = note: this mutable reference has lifetime `'static`, but if the static gets accessed (read or written) by any other means, or any other reference is created, then any further use of this mutable reference is Undefined Behavior
    = note: `#[warn(static_mut_refs)]` on by default
help: use `addr_of_mut!` instead to create a raw pointer
    |
473 |     pgrx::hooks::register_hook(addr_of_mut!(HOOKS));

Trying to rewrite that register_hook line to remove the warning, I ended up with the syntax below that seems to please cargo but looks fishy

        let h = &mut *std::ptr::addr_of_mut!(HOOKS) as &mut dyn PgHooks;                                                                         
        pgrx::hooks::register_hook(h);

I tried to understand the issue 114447, but it is way out of my league....

It seems obvious that we won't create another reference to the HOOKS variable, but is that enough to simply ignore this warning ?

@workingjubilee
Copy link
Member

workingjubilee commented Jul 5, 2024

We are going to move away from using static mut in pgrx. The use here is actually quite inappropriate and almost certainly UB, because we later do materialize references to it inside the various hooks:

pgrx/pgrx/src/hooks.rs

Lines 601 to 608 in 2c1d388

match HOOKS.as_mut().unwrap().prev_emit_log_hook.as_ref() {
None => (),
Some(f) => (f)(error_data.as_ptr()),
}
})
}
let hook = &mut HOOKS.as_mut().unwrap().current_hook;

So I don't think

It seems obvious that we won't create another reference to the HOOKS variable

is quite correct.

Fixing this requires a redesign of the API.

@workingjubilee
Copy link
Member

Trying to rewrite that register_hook line to remove the warning, I ended up with the syntax below that seems to please cargo but looks fishy

        let h = &mut *std::ptr::addr_of_mut!(HOOKS) as &mut dyn PgHooks;                                                                         
        pgrx::hooks::register_hook(h);

And you are correct that this is exactly 0% better, by the way.

@workingjubilee
Copy link
Member

workingjubilee commented Jul 5, 2024

I believe the initial "reborrow" of current_hook is not UB but the borrow we do inside prev of each specific prev_whatever_hook is?

It would probably be useful if someone created a "small-scale model" of the "triggering a hook" sequence of events, with a much-pared-back version of our hooks API, and ran it through Miri. I think it might be possible to fix the biggest hooks API UB with mostly-internal changes.

@daamien
Copy link
Contributor Author

daamien commented Jul 5, 2024

Thanks for the explanations

@EdMcBane
Copy link
Contributor

It would probably be useful if someone created a "small-scale model" of the "triggering a hook" sequence of events, with a much-pared-back version of our hooks API, and ran it through Miri. I think it might be possible to fix the biggest hooks API UB with mostly-internal changes.

I’d be up to try and do that.

As of 0.12, we are stuck with using an unsound and deprecated API with no alternative, as we need to intercept utility commands in our extension.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants