changeset 43208:1ca3823aeefd

rust-cpython: add wrapper around decapsule_make_dirstate_tuple() There are a couple of safety issues. First, the returned function pointer must be unsafe. Second, its return value must be a raw pointer (i.e. python27_sys::PyObject), not a cpython::PyObject. The wrapper function will address these issues.
author Yuya Nishihara <yuya@tcha.org>
date Sun, 13 Oct 2019 16:55:17 +0900
parents 4aa9f3a1c1df
children 8b355cbef16d
files rust/hg-cpython/src/dirstate.rs rust/hg-cpython/src/dirstate/dirstate_map.rs rust/hg-cpython/src/parsers.rs
diffstat 3 files changed, 30 insertions(+), 71 deletions(-) [+]
line wrap: on
line diff
--- a/rust/hg-cpython/src/dirstate.rs	Sun Oct 13 02:10:26 2019 +0200
+++ b/rust/hg-cpython/src/dirstate.rs	Sun Oct 13 16:55:17 2019 +0900
@@ -46,9 +46,7 @@
 /// This is largely a copy/paste from cindex.rs, pending the merge of a
 /// `py_capsule_fn!` macro in the rust-cpython project:
 /// https://github.com/dgrunwald/rust-cpython/pull/169
-pub fn decapsule_make_dirstate_tuple(
-    py: Python,
-) -> PyResult<MakeDirstateTupleFn> {
+fn decapsule_make_dirstate_tuple(py: Python) -> PyResult<MakeDirstateTupleFn> {
     unsafe {
         let caps_name = CStr::from_bytes_with_nul_unchecked(
             b"mercurial.cext.parsers.make_dirstate_tuple_CAPI\0",
@@ -61,6 +59,25 @@
     }
 }
 
+pub fn make_dirstate_tuple(
+    py: Python,
+    entry: &DirstateEntry,
+) -> PyResult<PyObject> {
+    let make = decapsule_make_dirstate_tuple(py)?;
+
+    let &DirstateEntry {
+        state,
+        mode,
+        size,
+        mtime,
+    } = entry;
+    // Explicitly go through u8 first, then cast to platform-specific `c_char`
+    // because Into<u8> has a specific implementation while `as c_char` would
+    // just do a naive enum cast.
+    let state_code: u8 = state.into();
+    Ok(make(state_code as c_char, mode, size, mtime))
+}
+
 pub fn extract_dirstate(py: Python, dmap: &PyDict) -> Result<StateMap, PyErr> {
     dmap.items(py)
         .iter()
--- a/rust/hg-cpython/src/dirstate/dirstate_map.rs	Sun Oct 13 02:10:26 2019 +0200
+++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs	Sun Oct 13 16:55:17 2019 +0900
@@ -16,11 +16,10 @@
     exc, ObjectProtocol, PyBool, PyBytes, PyClone, PyDict, PyErr, PyObject,
     PyResult, PyTuple, Python, PythonObject, ToPyObject,
 };
-use libc::c_char;
 
 use crate::{
     dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator},
-    dirstate::{decapsule_make_dirstate_tuple, dirs_multiset::Dirs},
+    dirstate::{dirs_multiset::Dirs, make_dirstate_tuple},
     ref_sharing::{PyLeakedRef, PySharedRefCell},
 };
 use hg::{
@@ -66,15 +65,7 @@
         let key = key.extract::<PyBytes>(py)?;
         match self.inner(py).borrow().get(HgPath::new(key.data(py))) {
             Some(entry) => {
-                // Explicitly go through u8 first, then cast to
-                // platform-specific `c_char`.
-                let state: u8 = entry.state.into();
-                Ok(Some(decapsule_make_dirstate_tuple(py)?(
-                        state as c_char,
-                        entry.mode,
-                        entry.size,
-                        entry.mtime,
-                    )))
+                Ok(Some(make_dirstate_tuple(py, entry)?))
             },
             None => Ok(default)
         }
@@ -303,15 +294,7 @@
         let key = HgPath::new(key.data(py));
         match self.inner(py).borrow().get(key) {
             Some(entry) => {
-                // Explicitly go through u8 first, then cast to
-                // platform-specific `c_char`.
-                let state: u8 = entry.state.into();
-                Ok(decapsule_make_dirstate_tuple(py)?(
-                        state as c_char,
-                        entry.mode,
-                        entry.size,
-                        entry.mtime,
-                    ))
+                Ok(make_dirstate_tuple(py, entry)?)
             },
             None => Err(PyErr::new::<exc::KeyError, _>(
                 py,
@@ -493,18 +476,9 @@
         res: (&HgPathBuf, &DirstateEntry),
     ) -> PyResult<Option<(PyBytes, PyObject)>> {
         let (f, entry) = res;
-
-        // Explicitly go through u8 first, then cast to
-        // platform-specific `c_char`.
-        let state: u8 = entry.state.into();
         Ok(Some((
             PyBytes::new(py, f.as_ref()),
-            decapsule_make_dirstate_tuple(py)?(
-                state as c_char,
-                entry.mode,
-                entry.size,
-                entry.mtime,
-            ),
+            make_dirstate_tuple(py, entry)?,
         )))
     }
 }
--- a/rust/hg-cpython/src/parsers.rs	Sun Oct 13 02:10:26 2019 +0200
+++ b/rust/hg-cpython/src/parsers.rs	Sun Oct 13 16:55:17 2019 +0900
@@ -15,15 +15,13 @@
     PythonObject, ToPyObject,
 };
 use hg::{
-    pack_dirstate, parse_dirstate, utils::hg_path::HgPathBuf, DirstateEntry,
+    pack_dirstate, parse_dirstate, utils::hg_path::HgPathBuf,
     DirstatePackError, DirstateParents, DirstateParseError, PARENT_SIZE,
 };
 use std::collections::HashMap;
 use std::convert::TryInto;
 
-use libc::c_char;
-
-use crate::dirstate::{decapsule_make_dirstate_tuple, extract_dirstate};
+use crate::dirstate::{extract_dirstate, make_dirstate_tuple};
 use std::time::Duration;
 
 fn parse_dirstate_wrapper(
@@ -37,22 +35,11 @@
 
     match parse_dirstate(&mut dirstate_map, &mut copies, st.data(py)) {
         Ok(parents) => {
-            for (filename, entry) in dirstate_map {
-                // Explicitly go through u8 first, then cast to
-                // platform-specific `c_char` because Into<u8> has a specific
-                // implementation while `as c_char` would just do a naive enum
-                // cast.
-                let state: u8 = entry.state.into();
-
+            for (filename, entry) in &dirstate_map {
                 dmap.set_item(
                     py,
                     PyBytes::new(py, filename.as_ref()),
-                    decapsule_make_dirstate_tuple(py)?(
-                        state as c_char,
-                        entry.mode,
-                        entry.size,
-                        entry.mtime,
-                    ),
+                    make_dirstate_tuple(py, entry)?,
                 )?;
             }
             for (path, copy_path) in copies {
@@ -127,30 +114,11 @@
         Duration::from_secs(now.as_object().extract::<u64>(py)?),
     ) {
         Ok(packed) => {
-            for (
-                filename,
-                DirstateEntry {
-                    state,
-                    mode,
-                    size,
-                    mtime,
-                },
-            ) in dirstate_map
-            {
-                // Explicitly go through u8 first, then cast to
-                // platform-specific `c_char` because Into<u8> has a specific
-                // implementation while `as c_char` would just do a naive enum
-                // cast.
-                let state: u8 = state.into();
+            for (filename, entry) in &dirstate_map {
                 dmap.set_item(
                     py,
                     PyBytes::new(py, filename.as_ref()),
-                    decapsule_make_dirstate_tuple(py)?(
-                        state as c_char,
-                        mode,
-                        size,
-                        mtime,
-                    ),
+                    make_dirstate_tuple(py, entry)?,
                 )?;
             }
             Ok(PyBytes::new(py, &packed))