Debug assert to verify the correctness at DeframerVecBuffer #1929
Conversation
| @@ -504,7 +504,8 @@ impl DeframerBuffer<'_, InternalPayload> for DeframerVecBuffer { | |||
|
|
|||
| #[cfg(feature = "std")] | |||
| impl<'a> DeframerBuffer<'a, ExternalPayload<'a>> for DeframerVecBuffer { | |||
| fn copy(&mut self, payload: &ExternalPayload<'a>, _at: usize) { | |||
| fn copy(&mut self, payload: &ExternalPayload<'a>, at: usize) { | |||
| debug_assert_eq!(at, self.used); | |||
There was a problem hiding this comment.
You also indicated you thought some of the context from the fix commit message deserved to be a comment. That sounds reasonable to me, could you roll that into this diff?
There was a problem hiding this comment.
Yes, I was thinking about that too. However it is not trivial how to formulate this context from a commit message explaining why this was changes into a more permanent why is it "ok to be like this" comment for the code.
While thinking about it - I figures that maybe this is actually not an ok thing to have - and a broader remodelling of how this trait and in general part of logic is in order.
If you can agree this is the case - that is what I'll capture and add as a comment.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1929 +/- ##
=======================================
Coverage 95.49% 95.49%
=======================================
Files 86 86
Lines 18650 18651 +1
=======================================
+ Hits 17810 17811 +1
Misses 840 840 ☔ View full report in Codecov by Sentry. |
|
Actually, is this even correct? it seems like the |
|
Thanks for the contribution. I chatted about this branch with the other maintainers and we've reached the conclusion we'd like to close this for now. As mentioned in the other thread we would be supportive of a broader refactor to address some of the awkward workarounds that remain in this area. Outside of that, if you have other concerns about the fix we landed it would be beneficial to have them articulated in a separate issue, ideally with a concrete case in which the existing fix can be demonstrated as insufficient. |
|
A quick question here though: in your discussion, did you conclude this invariant check that I have added wouldn't hold? Cause if that's the case then there is likely a corruption possible here that might be exploited as a vulnerability. |
|
We believe the invariant holds. |
Follow up after #1798 (comment)