| Submitter | phabricator |
|---|---|
| Date | Jan. 6, 2020, 7:26 p.m. |
| Message ID | <differential-rev-PHID-DREV-3mwbbura2ccoppyytg3h-req@mercurial-scm.org> |
| Download | mbox | patch |
| Permalink | /patch/44156/ |
| State | New |
| Headers | show |
Comments
This revision now requires changes to proceed. kevincox added inline comments. kevincox requested changes to this revision. INLINE COMMENTS > nodemap.rs:268 > + Vec::from_raw_parts( > + vec.as_ptr() as *mut u8, > + vec.len() * BLOCK_SIZE, This makes me uncomfortable. There is really no guarantee that there is no padding around `Block` or `RawElement`. If there is any padding this is UB. I would much prefer one of: 1. Store growable as a set of bytes and convert on get/set. 2. Add a method that outputs the bytes to any `Write`. In theory this will be slower but it is probably immaterial (especially if there is no padding). > nodemap.rs:272 > + ) > + }; > + mem::forget(vec); If someone panics between the `from_raw_parts` and `mem::forget` this is a double free. Right now I think this can't happen but it is completely possible that the code changes between then and now. I would prefer using `Vec::from_raw_parts` to make sure that there is only one `Vec` alive with that pointer at a time. This risks a leak in the face of panic but I think that is much better. > nodemap.rs:433 > + amount: usize, > + ) -> Self { > + assert!(buffer.len() >= offset + amount); This is a weird API. Why does new take a buffer and an offset? Can it take either a slice or just remove the offset parameter and always have the buffer start at 0? It is very strange to be passing in data that we own but isn't ever used. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7796/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7796 To: gracinet, #hg-reviewers, kevincox Cc: durin42, kevincox, mercurial-devel
kevincox added inline comments. INLINE COMMENTS > kevincox wrote in nodemap.rs:268 > This makes me uncomfortable. There is really no guarantee that there is no padding around `Block` or `RawElement`. If there is any padding this is UB. I would much prefer one of: > > 1. Store growable as a set of bytes and convert on get/set. > 2. Add a method that outputs the bytes to any `Write`. In theory this will be slower but it is probably immaterial (especially if there is no padding). I thought about this more and I think I am okay doing it this way. It seems like this should be well defined as long as there is no padding. However on that note I would want to add a check that there is no padding as expected. I would also like to ensure that this fails to compile if there is ever padding, even in release mode. I think this can be accomplished by something like: let _: [u8; 4 * BLOCK_SIZE] = std::mem::transmute([Block::new(); 4]); I would probably want to repeat this check near any code that is relying on this invariant. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7796/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7796 To: gracinet, #hg-reviewers, kevincox Cc: durin42, kevincox, mercurial-devel
gracinet added a comment. @kevincox (replying hastily because I'm also finishing something unrelated today). Doesn't `mem::size_of` guarantee to take any padding into account? At least that's what the doc seems to say: https://doc.rust-lang.org/std/mem/fn.size_of.html REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7796/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7796 To: gracinet, #hg-reviewers, kevincox Cc: durin42, kevincox, mercurial-devel
kevincox added a comment. Ah yes. So my test would be ineffective. The problem that IIUC padding is not guaranteed to be initialized, and in rust all values need to be initialized. So if you cast a type with padding to a `u8` it is undefined behaviour. So we would need a test that shows that the type is the same size as the sum of the contained `i32`s. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7796/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7796 To: gracinet, #hg-reviewers, kevincox Cc: durin42, kevincox, mercurial-devel
kevincox added a comment. Maybe what we should do is keep the test I proposed, but change the definition of `BLOCK_SIZE` to be based upon the number of contained `i32` instead of `size_of`. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7796/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7796 To: gracinet, #hg-reviewers, kevincox Cc: durin42, kevincox, mercurial-devel
Patch
diff --git a/rust/hg-core/src/revlog/nodemap.rs b/rust/hg-core/src/revlog/nodemap.rs --- a/rust/hg-core/src/revlog/nodemap.rs +++ b/rust/hg-core/src/revlog/nodemap.rs @@ -17,8 +17,10 @@ RevlogIndex, }; use std::fmt; +use std::mem; use std::ops::Deref; use std::ops::Index; +use std::slice; #[derive(Debug, PartialEq)] pub enum NodeMapError { @@ -132,6 +134,8 @@ #[derive(Clone, PartialEq)] pub struct Block([RawElement; 16]); +pub const BLOCK_SIZE: usize = mem::size_of::<Block>(); + impl Block { fn new() -> Self { Block([-1; 16]) @@ -221,6 +225,57 @@ } } + /// Create from an opaque bunch of bytes + /// + /// The created `NodeTreeBytes` is taken after the fixed `offset` from + /// `buffer`, of which exactly `amount` bytes are used. + /// + /// - `buffer` could be derived from `PyBuffer` and `Mmap` objects. + /// - `offset` allows for the final file format to include fixed data + /// (generation number, behavioural flags) + /// - `amount` is expressed in bytes, and is not automatically derived from + /// `bytes`, so that a caller that manages them atomically can perform + /// temporary disk serializations and still rollback easily if needed. + /// First use-case for this would be to support Mercurial shell hooks. + /// + /// panics if `buffer` is smaller than `offset + amount` + pub fn load_bytes( + bytes: Box<dyn Deref<Target = [u8]> + Send>, + offset: usize, + amount: usize, + ) -> Self { + NodeTree::new(Box::new(NodeTreeBytes::new(bytes, offset, amount))) + } + + /// Retrieve added `Block` and the original immutable data + pub fn into_readonly_and_added( + self, + ) -> (Box<dyn Deref<Target = [Block]> + Send>, Vec<Block>) { + let mut vec = self.growable; + let readonly = self.readonly; + if readonly.last() != Some(&self.root) { + vec.push(self.root); + } + (readonly, vec) + } + + /// Retrieve added `Blocks` as bytes, ready to be written to persistent + /// storage + pub fn into_readonly_and_added_bytes( + self, + ) -> (Box<dyn Deref<Target = [Block]> + Send>, Vec<u8>) { + let (readonly, vec) = self.into_readonly_and_added(); + let bytes = unsafe { + Vec::from_raw_parts( + vec.as_ptr() as *mut u8, + vec.len() * BLOCK_SIZE, + vec.capacity() * BLOCK_SIZE, + ) + }; + mem::forget(vec); + (readonly, bytes) + } + /// Total number of blocks fn len(&self) -> usize { self.readonly.len() + self.growable.len() + 1 @@ -366,6 +421,42 @@ } } +pub struct NodeTreeBytes { + buffer: Box<dyn Deref<Target = [u8]> + Send>, + offset: usize, + len_in_blocks: usize, +} + +impl NodeTreeBytes { + fn new( + buffer: Box<dyn Deref<Target = [u8]> + Send>, + offset: usize, + amount: usize, + ) -> Self { + assert!(buffer.len() >= offset + amount); + let len_in_blocks = amount / BLOCK_SIZE; + NodeTreeBytes { + buffer, + offset, + len_in_blocks, + } + } +} + +impl Deref for NodeTreeBytes { + type Target = [Block]; + + fn deref(&self) -> &[Block] { + unsafe { + slice::from_raw_parts( + (&self.buffer).as_ptr().offset(self.offset as isize) + as *const Block, + self.len_in_blocks, + ) + } + } +} + struct NodeTreeVisitor<'n, 'p> { nt: &'n NodeTree, prefix: NodePrefixRef<'p>, @@ -710,4 +801,31 @@ Ok(()) } + + #[test] + fn test_into_added_empty() { + assert!(sample_nodetree().into_readonly_and_added().1.is_empty()); + assert!(sample_nodetree() + .into_readonly_and_added_bytes() + .1 + .is_empty()); + } + + #[test] + fn test_into_added_bytes() -> Result<(), NodeMapError> { + let mut idx = TestNtIndex::new(); + idx.insert(0, "1234")?; + let mut idx = idx.commit(); + idx.insert(4, "cafe")?; + let (_, bytes) = idx.nt.into_readonly_and_added_bytes(); + + // only the root block has been changed + assert_eq!(bytes.len(), BLOCK_SIZE); + // big endian for -2 + assert_eq!(&bytes[4..2 * 4], [255, 255, 255, 254]); + // big endian for -6 + assert_eq!(&bytes[12 * 4..13 * 4], [255, 255, 255, 250]); + Ok(()) + } + }