[libvirt] [libvirt-rust PATCH 1/1] Make creating safe wrapper from raw pointer unsafe

Giving an invalid pointer to the safe wrapper types causes undefined behavior when methods are later called on said wrapper Properly document safety contract of using unsafe constructor --- src/connect.rs | 10 ++++++++-- src/domain.rs | 10 ++++++++-- src/domain_snapshot.rs | 10 ++++++++-- src/interface.rs | 10 ++++++++-- src/network.rs | 10 ++++++++-- src/nodedev.rs | 10 ++++++++-- src/nwfilter.rs | 10 ++++++++-- src/secret.rs | 10 ++++++++-- src/storage_pool.rs | 10 ++++++++-- src/storage_vol.rs | 10 ++++++++-- src/stream.rs | 8 +++++++- 11 files changed, 87 insertions(+), 21 deletions(-) diff --git a/src/connect.rs b/src/connect.rs index 108224d..7f02619 100644 --- a/src/connect.rs +++ b/src/connect.rs @@ -462,8 +462,14 @@ impl Connect { self.ptr.unwrap() } - pub fn new(ptr: sys::virConnectPtr) -> Connect { - return Connect { ptr: Some(ptr) }; + /// Creates a new [Connect] from the given raw pointer. + /// + /// # Safety + /// + /// The pointer must point to a valid libvirt connection object. + /// The underlying object must be valid for the entire lifetime of the returned instance. + pub unsafe fn from_ptr(ptr: sys::virConnectPtr) -> Connect { + Connect { ptr: Some(ptr) } } pub fn get_version() -> Result<u32, Error> { diff --git a/src/domain.rs b/src/domain.rs index acb9e6e..0df95ee 100644 --- a/src/domain.rs +++ b/src/domain.rs @@ -628,8 +628,14 @@ impl Drop for Domain { } impl Domain { - pub fn new(ptr: sys::virDomainPtr) -> Domain { - return Domain { ptr: Some(ptr) }; + /// Creates a new [Domain] from the given raw pointer. + /// + /// # Safety + /// + /// The pointer must point to a valid libvirt domain object. + /// The underlying object must be valid for the entire lifetime of the returned instance. + pub unsafe fn from_ptr(ptr: sys::virDomainPtr) -> Domain { + Domain { ptr: Some(ptr) } } pub fn as_ptr(&self) -> sys::virDomainPtr { diff --git a/src/domain_snapshot.rs b/src/domain_snapshot.rs index 86599ef..8b6c873 100644 --- a/src/domain_snapshot.rs +++ b/src/domain_snapshot.rs @@ -94,8 +94,14 @@ impl Drop for DomainSnapshot { } impl DomainSnapshot { - pub fn new(ptr: sys::virDomainSnapshotPtr) -> DomainSnapshot { - return DomainSnapshot { ptr: Some(ptr) }; + /// Creates a new [DomainSnapshot] from the given raw pointer. + /// + /// # Safety + /// + /// The pointer must point to a valid libvirt domain snapshot object. + /// The underlying object must be valid for the entire lifetime of the returned instance. + pub unsafe fn from_ptr(ptr: sys::virDomainSnapshotPtr) -> DomainSnapshot { + DomainSnapshot { ptr: Some(ptr) } } pub fn as_ptr(&self) -> sys::virDomainSnapshotPtr { diff --git a/src/interface.rs b/src/interface.rs index 6fe8742..b06b641 100644 --- a/src/interface.rs +++ b/src/interface.rs @@ -81,8 +81,14 @@ impl Drop for Interface { } impl Interface { - pub fn new(ptr: sys::virInterfacePtr) -> Interface { - return Interface { ptr: Some(ptr) }; + /// Creates a new [Interface] from the given raw pointer. + /// + /// # Safety + /// + /// The pointer must point to a valid libvirt interface object. + /// The underlying object must be valid for the entire lifetime of the returned instance. + pub unsafe fn from_ptr(ptr: sys::virInterfacePtr) -> Interface { + Interface { ptr: Some(ptr) } } pub fn as_ptr(&self) -> sys::virInterfacePtr { diff --git a/src/network.rs b/src/network.rs index ac8c042..bbe5539 100644 --- a/src/network.rs +++ b/src/network.rs @@ -117,8 +117,14 @@ impl Drop for Network { } impl Network { - pub fn new(ptr: sys::virNetworkPtr) -> Network { - return Network { ptr: Some(ptr) }; + /// Creates a new [Network] from the given raw pointer. + /// + /// # Safety + /// + /// The pointer must point to a valid libvirt network object. + /// The underlying object must be valid for the entire lifetime of the returned instance. + pub unsafe fn from_ptr(ptr: sys::virNetworkPtr) -> Network { + Network { ptr: Some(ptr) } } pub fn as_ptr(&self) -> sys::virNetworkPtr { diff --git a/src/nodedev.rs b/src/nodedev.rs index c38c49e..2f36ea6 100644 --- a/src/nodedev.rs +++ b/src/nodedev.rs @@ -99,8 +99,14 @@ impl Drop for NodeDevice { } impl NodeDevice { - pub fn new(ptr: sys::virNodeDevicePtr) -> NodeDevice { - return NodeDevice { ptr: Some(ptr) }; + /// Creates a new [NodeDevice] from the given raw pointer. + /// + /// # Safety + /// + /// The pointer must point to a valid libvirt node device object. + /// The underlying object must be valid for the entire lifetime of the returned instance. + pub unsafe fn from_ptr(ptr: sys::virNodeDevicePtr) -> NodeDevice { + NodeDevice { ptr: Some(ptr) } } pub fn as_ptr(&self) -> sys::virNodeDevicePtr { diff --git a/src/nwfilter.rs b/src/nwfilter.rs index bd9455d..84f440e 100644 --- a/src/nwfilter.rs +++ b/src/nwfilter.rs @@ -68,8 +68,14 @@ impl Drop for NWFilter { } impl NWFilter { - pub fn new(ptr: sys::virNWFilterPtr) -> NWFilter { - return NWFilter { ptr: Some(ptr) }; + /// Creates a new [NWFilter] from the given raw pointer. + /// + /// # Safety + /// + /// The pointer must point to a valid libvirt NWFilter object. + /// The underlying object must be valid for the entire lifetime of the returned instance. + pub unsafe fn from_ptr(ptr: sys::virNWFilterPtr) -> NWFilter { + NWFilter { ptr: Some(ptr) } } pub fn as_ptr(&self) -> sys::virNWFilterPtr { diff --git a/src/secret.rs b/src/secret.rs index ba96b02..59a3f19 100644 --- a/src/secret.rs +++ b/src/secret.rs @@ -100,8 +100,14 @@ impl Drop for Secret { } impl Secret { - pub fn new(ptr: sys::virSecretPtr) -> Secret { - return Secret { ptr: Some(ptr) }; + /// Creates a new [Secret] from the given raw pointer. + /// + /// # Safety + /// + /// The pointer must point to a valid libvirt secret object. + /// The underlying object must be valid for the entire lifetime of the returned instance. + pub unsafe fn from_ptr(ptr: sys::virSecretPtr) -> Secret { + Secret { ptr: Some(ptr) } } pub fn as_ptr(&self) -> sys::virSecretPtr { diff --git a/src/storage_pool.rs b/src/storage_pool.rs index 38676c2..b8d9feb 100644 --- a/src/storage_pool.rs +++ b/src/storage_pool.rs @@ -156,8 +156,14 @@ impl Drop for StoragePool { } impl StoragePool { - pub fn new(ptr: sys::virStoragePoolPtr) -> StoragePool { - return StoragePool { ptr: Some(ptr) }; + /// Creates a new [StoragePool] from the given raw pointer. + /// + /// # Safety + /// + /// The pointer must point to a valid libvirt storage pool object. + /// The underlying object must be valid for the entire lifetime of the returned instance. + pub unsafe fn from_ptr(ptr: sys::virStoragePoolPtr) -> StoragePool { + StoragePool { ptr: Some(ptr) } } pub fn as_ptr(&self) -> sys::virStoragePoolPtr { diff --git a/src/storage_vol.rs b/src/storage_vol.rs index afc9b17..762c615 100644 --- a/src/storage_vol.rs +++ b/src/storage_vol.rs @@ -181,8 +181,14 @@ impl Drop for StorageVol { } impl StorageVol { - pub fn new(ptr: sys::virStorageVolPtr) -> StorageVol { - return StorageVol { ptr: Some(ptr) }; + /// Creates a new [StorageVol] from the given raw pointer. + /// + /// # Safety + /// + /// The pointer must point to a valid libvirt storage volume object. + /// The underlying object must be valid for the entire lifetime of the returned instance. + pub unsafe fn from_ptr(ptr: sys::virStorageVolPtr) -> StorageVol { + StorageVol { ptr: Some(ptr) } } pub fn as_ptr(&self) -> sys::virStorageVolPtr { diff --git a/src/stream.rs b/src/stream.rs index 8333ee5..ff85bea 100644 --- a/src/stream.rs +++ b/src/stream.rs @@ -68,7 +68,13 @@ impl Drop for Stream { } impl Stream { - pub fn new(ptr: sys::virStreamPtr) -> Stream { + /// Creates a new [Stream] from the given raw pointer. + /// + /// # Safety + /// + /// The pointer must point to a valid libvirt stream object. + /// The underlying object must be valid for the entire lifetime of the returned instance. + pub unsafe fn from_ptr(ptr: sys::virStreamPtr) -> Stream { Stream { ptr: Some(ptr) } } -- 2.21.0

On Wed, Sep 25, 2019 at 04:36:16PM +0000, Linus Färnstrand wrote:
Giving an invalid pointer to the safe wrapper types causes undefined behavior when methods are later called on said wrapper
Properly document safety contract of using unsafe constructor ---
Ideally it should not be exposed at all because there already is a method for getting the proper structure. And that method (or those methods) already use ::new() so with this patch it just fails. Tests, examples, etc. Instead of fixing that I would, probably, just not expose them. I don't see a reason for them being public.

On Mon, Sep 30, 2019 at 12:10:49PM +0200, Martin Kletzander wrote:
On Wed, Sep 25, 2019 at 04:36:16PM +0000, Linus Färnstrand wrote:
Giving an invalid pointer to the safe wrapper types causes undefined behavior when methods are later called on said wrapper
Properly document safety contract of using unsafe constructor ---
Ideally it should not be exposed at all because there already is a method for getting the proper structure. And that method (or those methods) already use ::new() so with this patch it just fails. Tests, examples, etc. Instead of fixing that I would, probably, just not expose them. I don't see a reason for them being public.
Yes I'm agree, they should just not be public. s.
participants (3)
-
Linus Färnstrand
-
Martin Kletzander
-
Sahid Orentino Ferdjaoui