unwind safety fixes - #140
Conversation
235fcfb to
45e29c0
Compare
7ad7985 to
225f13b
Compare
e45f41e to
e3b2cb6
Compare
e3b2cb6 to
e5f75a9
Compare
|
I cannot review diff of diffs. Please always squash your changes to individually meaningful commits. |
e5f75a9 to
5c83836
Compare
|
I see, everything has been squashed to the original three commits. When reviewing, I prefer having diffs that do exactly what each of the comments suggests and squashing after the review. I will squash future fixups immediately. |
|
For GitHub-centric workflow that's indeed convenient, but for this project as the commits are synced to the kernel I need to review each commit individually so it's up to kernel's quality requirement. Honestly the actual issue is that GitHub does not provide a good way to visualize the diff, despite that it has all the needed information to render as such. The "Compare" button sometimes does the job, but it breaks when you rebase and fixup commits in a single push. Longer term, either we can wait until GitHub implements it (which is quite unlikely given the slop), or we can adopt what the Rust Project's triagebot does by automatically generates a range diff like this. |
|
I understand and fully agree. Unfortunately, looking at recent trends at GitHub, I don't expect much in terms of improvements to the platform. Thank you for taking the time to explain your reasoning. :) |
5c83836 to
eebe948
Compare
eebe948 to
8b46ef9
Compare
nbdd0121
left a comment
There was a problem hiding this comment.
Thanks for working on this. Some further feedbacks.
Also, could you move the ArrayInit to lib.rs just before init_array_from_fn? The __internal module is supposed to only host items that need to be public due to the use of generated code from pin-init-internal (yes, we have a few pub(crate) there, but that's for historical reasons and I'm working on moving them).
8b46ef9 to
59950ff
Compare
|
Apologies for the delay.
Thank you! I really appreciate the level of detail in your review/feedback. It is rare to come by these days and I'm really glad to work with people that care this much about the code they maintain.
Done. Also thank you for the clarification on what should go where, I tried to guess based on types like I will respond to individual comments for the rest of your feedback. |
|
Yeah, |
nbdd0121
left a comment
There was a problem hiding this comment.
kernel's checkpatch.pl gives the following warnings:
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
4610dc9 to
29d9bcd
Compare
I could not find any report from LKML to use in the |
|
|
|
Clippy 1.85.0 reports an false-positive: |
|
Thank you for the feedback. I am very sorry about the delay, last week was very busy for me. I will do my best to have an update here this week. |
No rush! Thank you for working on this. |
|
@Mirko-A are you still interested in working on this? If not, I can also finish it off myself building on top of yours. |
|
@nbdd0121 Please finish it off, I couldn't find enough spare time. I hope the work I did was of some help at least. |
29d9bcd to
48f3139
Compare
The previous code only ran cleanup on the explicit error path. If the per- element initializer panicked partway through, the elements already written into the array would be leaked: their `Drop` impls would never run. This violates the pinning requirement. Fix the unwind safety issue by adding a guard type that drops element on both error and panic path. To avoid having to duplicate code between `pin_init_array_from_fn` and the non-pin variant, extract the code to a shared `ArrayInit` type; this type is internal and not visible via API. Reported-by: Gary Guo <gary@garyguo.net> Closes: Rust-for-Linux#136 Signed-off-by: Mirko Adzic <adzicmirko97@gmail.com> [ Split guard type and the initializer type, move the guard type to be within __pinned_init. - Gary ] Co-developed-by: Gary Guo <gary@garyguo.net> Signed-off-by: Gary Guo <gary@garyguo.net>
Add a drop guard before the call to the chained closure so that the value initialized by the first stage is dropped if the closure errors or panics; `mem::forget` the guard on success. The previous code only ran cleanup on the explicit error path, leaking the first-stage value if the chained closure panicked. Reported-by: Gary Guo <gary@garyguo.net> Closes: Rust-for-Linux#136 Suggested-by: Gary Guo <gary@garyguo.net> Signed-off-by: Mirko Adzic <adzicmirko97@gmail.com> [ Fix Clippy missing safety comment false positive when `slot` and `guard` creation are merged in a single line. - Gary ] Signed-off-by: Gary Guo <gary@garyguo.net>
Add test for `[pin_]init_array_from_fn` and `[pin_]chain` that tests various error and panicking cases to ensure safety. Also assert no double-drop on the success paths. Signed-off-by: Mirko Adzic <adzicmirko97@gmail.com> [ Code cleanups. - Gary ] Signed-off-by: Gary Guo <gary@garyguo.net>
48f3139 to
112aa13
Compare
Adds unwind-safety fixes to in-place initialization helpers so partially initialized values are dropped on both errors and panics. Changes:
__internal::ArrayInitguard type that safely initializes an array by running an initializer on each element, keeping track of the number of initialized elements. When dropped, if the initialization was partially successful, the guard drops the initialized portion of the array. Use the guard in[pin_]init_array_from_fn.__internal::DropGuardin[pin_]chainso a panic/error in the chained closure drops the value initialized in the first stage;mem::forgetthe guard on success.Closes: #136
The approach taken here was suggested in the issue itself, thus I added a
Suggested-bytag. I hope that is okay.Edits:
v1 -> v2:
ArrayInitGuardtoArrayInit[Pin]InitforArrayInitinstead of wrapping it in[pin_]init_from_closurev2 -> v3:
mem::forgettheArrayInitguard on success; state that the guard will only drop initialized elements on partial success