D7796: rust-nodemap: input/output primitives

kevincox (Kevin Cox) phabricator at mercurial-scm.org
Thu Jan 16 06:24:04 EST 2020

This revision now requires changes to proceed.
kevincox added inline comments.
kevincox requested changes to this revision.


> 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.

  rHG Mercurial



To: gracinet, #hg-reviewers, kevincox
Cc: durin42, kevincox, mercurial-devel

More information about the Mercurial-devel mailing list