fix: Do not alias fields of tracked_struct Values when updating
#741
Conversation
✅ Deploy Preview for salsa-rs canceled.
|
src/tracked_struct/tracked_field.rs
Outdated
| @@ -62,7 +62,7 @@ where | |||
| revision: crate::Revision, | |||
| ) -> MaybeChangedAfter { | |||
| let zalsa = db.zalsa(); | |||
| let data = <super::IngredientImpl<C>>::data(zalsa.table(), input); | |||
| let data = <super::IngredientImpl<C>>::data(zalsa.table(), input, zalsa.current_revision()); | |||
There was a problem hiding this comment.
This now takes the read lock as it ought to from what I understand. It reads the revisions field which may be mutable accessed by update
There was a problem hiding this comment.
Discussion from meeting: I don't think the lock has to cover everything, I think it is only needed for the actual field values, not the metadata, which would permit these values to be factored
There was a problem hiding this comment.
Thinking about it more, I think -- it is correct to get the read-lock but if everyone is using it correctly, the lock should have already been acquired, because if the field may have changed, the creating query should have re-executed.
Veykril
force-pushed
the
veykril/push-ooqmumwlopur
branch
from
9bfedd4 to
0cf0cc7
Compare
CodSpeed Performance ReportMerging #741 will not alter performanceComparing Summary
|
Veykril
changed the title
tracked_struct Values when updating
| // the `mut`. We have now *claimed* this data by swapping in `None`, | ||
| // any attempt to read concurrently will panic. | ||
| let data = unsafe { &mut *data_raw }; |
There was a problem hiding this comment.
This was technically invalid before as it would alias the updated_at field
|
Does this fix an unsoundness because it otherwise seems to regress perf a little bit. |
|
I believe (if I understand the code correctly) this fixes a theoretical unsoundness when a user leaks a tracked struct value across threads. In that scenario we might try to acquire the read lock and write lock at the same time which could cause aliasing (before we inevitably panic). The perf regression likely comes from taking the read lock in |
|
I'll see if we can craft a test that actually hits that lock setup resulting in a panic (to check if miri might be able to diagnose the theoretical issue) |
Veykril
force-pushed
the
veykril/push-ooqmumwlopur
branch
from
8a61975 to
aa3446d
Compare
| // Observing `Some(current_revision)` can happen in two scenarios: leaks (tsk tsk) | ||
| // but also the scenario embodied by the test test `test_run_5_then_20` in `specify_tracked_fn_in_rev_1_but_not_2.rs`: | ||
| // Observing `Some(current_revision)` can happen in two scenarios: | ||
| // - leaks, see tests\preverify-struct-with-leaked-data.rs and tests\preverify-struct-with-leaked-data-2.rs | ||
| // - or the following scenario (FIXME verify this? There is no test that covers this behavior): |
There was a problem hiding this comment.
I am a bit curious about this second scenario. The test (test_run_5_then_20 in specify_tracked_fn_in_rev_1_but_not_2.rs) that used to exist here (its gone now) does not actually hit this path anymore. So something within salsa's behavior must've changed here, @nikomatsakis any idea? Is this scenario working out differently nowadays, given you removed the test in one of your PRs? The comment was added in 188f759#diff-545b847b1fec894075887ca68e52e020a61bc8f8e3d3ec6fe1bcbc27bb46b5d5 and the test was removed in nikomatsakis@3d2b2d3
Veykril
force-pushed
the
veykril/push-ooqmumwlopur
branch
2 times, most recently
from
b9b5e91 to
ba4f5c6
Compare
| // SAFETY: FIXME: why can this not alias with tracked_field::maybe_changed_after? | ||
| let revisions = unsafe { &mut (*data_raw).revisions }; | ||
|
|
||
| // SAFETY: We assert that the pointer to `data.revisions` | ||
| // is a pointer into the database referencing a value | ||
| // from a previous revision. As such, it continues to meet | ||
| // its validity invariant and any owned content also continues | ||
| // to meet its safety invariant. | ||
| let updated = unsafe { C::update_fields(current_revision, revisions, old_fields, fields) }; | ||
|
|
||
| // SAFETY: The mutable accesses to the following metadata fields will not alias | ||
| // as attempting to read any of these would first verify whether we are up to date, either | ||
| // blocking on our update or implying we would not have ended up in this method here in the | ||
| // first place. | ||
| // FIXME: why can these not alias | ||
| // - `durability`: with `tracked_field` / `untracked_field` | ||
| // - `revisions`: with `tracked_field::maybe_changed_after` | ||
| // - `created_at`: with `untracked_field` / `maybe_changed_after` |
There was a problem hiding this comment.
These I still need to check as to why we can say they are not going to end up aliasing (as Niko was talking about). I believe that is correct for all of them except for revisions, as that one is used to check if a field is outdated after all. Correct me if I am wrong but we first check that field to tell if we need to update the struct no? In which case we are in fact aliasing on that field
Veykril
force-pushed
the
veykril/push-ooqmumwlopur
branch
from
ba4f5c6 to
b0680ac
Compare
Veykril
changed the title
tracked_struct Values when updatingtracked_struct Values when updating
No description provided.