Skip to content

ExceptionZoo holds static ref #3717

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

Merged
merged 2 commits into from
May 27, 2022
Merged

Conversation

youknowone
Copy link
Member

No description provided.

@youknowone youknowone force-pushed the exception-ref branch 2 times, most recently from 611d858 to 6a226f8 Compare May 17, 2022 22:38
@coolreader18
Copy link
Member

Should it maybe be &'static Py<PyType> ? Just cause then there's less indirection. Though that's probably another round of bulk editing

@youknowone
Copy link
Member Author

youknowone commented May 17, 2022

Maybe PyStatic<T>? then PyStrInterned can be simply alias of PyStatic<PyStr>

@coolreader18
Copy link
Member

IMO if PyStatic is just a wrapper around &'static Py<T> I don't think that'd be necessary

@youknowone
Copy link
Member Author

PyStatic will be a wrapper of Py<T> but always intended to be used like &'static PyStatic<T>. It is not very different but overriding eq and Debug

https://github.com/RustPython/RustPython/blob/main/vm/src/intern.rs#L170-L197

@youknowone
Copy link
Member Author

Even if we are going to use PyStatic, finishing #3700 first and working on top of it will be better.

@youknowone
Copy link
Member Author

@coolreader18 changing to &Py<PyType> is not easy for now. This is related to PyPayload::class() -> &PyTypeRef and casting &'static Py<PyType> to PyTypeRef without a wrapper type like PyStatic will be messy.

@youknowone youknowone force-pushed the exception-ref branch 4 times, most recently from 8a833dc to 4771f93 Compare May 18, 2022 20:18
@youknowone
Copy link
Member Author

idk why but performance drops a lot by this change

@youknowone youknowone force-pushed the exception-ref branch 4 times, most recently from 784246b to 72020ea Compare May 19, 2022 04:57
@coolreader18
Copy link
Member

coolreader18 commented May 19, 2022

casting &'static Py<PyType> to PyTypeRef without a wrapper type like PyStatic will be messy.

Why? And I don't think that cast is sound, unless it does a check like is_leaked() and return Option<PyRef>

let pyref = ...;
let static_ref: &'static _ = PyStatic::leak(pyref);
let uaf = static_ref.cast_to_pyref();
let uaf2 = static_ref.cast_to_pyref();

@coolreader18
Copy link
Member

And like, can't the current fn class() -> &PyTypeRef just turn to fn class() -> &'static Py<PyType> by deref-coercing it? just let x: &'static Py<PyType> = /* &'static PyTypeRef */

@youknowone
Copy link
Member Author

youknowone commented May 19, 2022

I was misunderstanding something. When we have &Py<T>, it means it is always referring a valid PyRef<T>, right? So &Py<T> can have to_owned as it is. PyStatic is useless and it is just same to Py. Maybe this is what you meant.

What will be the problem of the example code? It doesn't seem any uaf happen there. If PyRef<T> is &Py<T>, getting &PyRef<T> is free by &&Py<T>

I didn't get which function do you want to refer by fn class() -> &PyTypeRef. more hint?

@youknowone youknowone force-pushed the exception-ref branch 4 times, most recently from 2cce181 to bec34fc Compare May 23, 2022 08:08
@youknowone youknowone requested a review from coolreader18 May 24, 2022 02:06
@youknowone youknowone merged commit 8ab4e77 into RustPython:main May 27, 2022
@youknowone youknowone deleted the exception-ref branch May 27, 2022 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants