From 039b070d1a709e06755951b53569dfec8fceaf4a Mon Sep 17 00:00:00 2001
From: Faruk Kasumovic <fkasumovic@gmail.com>
Date: Wed, 9 Sep 2020 10:22:37 +0200
Subject: [PATCH] - Prevent double-free failure when the same dataset is closed
 multiple times

In some cases, copying dataset object, either by passing by value or in
some other pattern can lead to code that will close the same dataset
multiple times (e.g. defer). Or otherwise, code with complicated
handling of dataset cleanup.
Fixes: #30, Closes: #31
---
 a_test.go       |  1 +
 destroy_test.go |  1 +
 zfs.go          | 29 +++++++++++++++++++----------
 zfs_test.go     | 32 ++++++++++++++++++++++++++++++++
 zpool_test.go   | 12 +++++++++---
 5 files changed, 62 insertions(+), 13 deletions(-)

diff --git a/a_test.go b/a_test.go
index ecef1b8..181dbaa 100644
--- a/a_test.go
+++ b/a_test.go
@@ -34,6 +34,7 @@ func Test(t *testing.T) {
 	zfsTestDatasetSetProperty(t)
 	zfsTestDatasetHoldRelease(t)
 
+	zfsTestDoubleFreeOnDestroy(t)
 	zfsTestDatasetDestroy(t)
 
 	zpoolTestPoolDestroy(t)
diff --git a/destroy_test.go b/destroy_test.go
index 7874a92..df771df 100644
--- a/destroy_test.go
+++ b/destroy_test.go
@@ -60,4 +60,5 @@ func TestDataset_DestroyPromote(t *testing.T) {
 	t.Log("Destroy promote completed with success")
 	d.Close()
 	zpoolTestPoolDestroy(t)
+	cleanupVDisks()
 }
diff --git a/zfs.go b/zfs.go
index 18de99d..31c84bc 100644
--- a/zfs.go
+++ b/zfs.go
@@ -13,6 +13,7 @@ import (
 	"path"
 	"sort"
 	"strings"
+	"sync"
 	"time"
 	"unsafe"
 )
@@ -49,6 +50,7 @@ type HoldTag struct {
 // Dataset - ZFS dataset object
 type Dataset struct {
 	list       C.dataset_list_ptr
+	closeOnce  *sync.Once
 	Type       DatasetType
 	Properties map[Prop]Property
 	Children   []Dataset
@@ -58,7 +60,7 @@ func (d *Dataset) openChildren() (err error) {
 	d.Children = make([]Dataset, 0, 5)
 	list := C.dataset_list_children(d.list)
 	for list != nil {
-		dataset := Dataset{list: list}
+		dataset := Dataset{list: list, closeOnce: new(sync.Once)}
 		dataset.Type = DatasetType(C.dataset_type(list))
 		dataset.Properties = make(map[Prop]Property)
 		err = dataset.ReloadProperties()
@@ -79,16 +81,20 @@ func (d *Dataset) openChildren() (err error) {
 // DatasetOpenAll recursive get handles to all available datasets on system
 // (file-systems, volumes or snapshots).
 func DatasetOpenAll() (datasets []Dataset, err error) {
-	var dataset Dataset
-	dataset.list = C.dataset_list_root()
-	for dataset.list != nil {
-		dataset.Type = DatasetType(C.dataset_type(dataset.list))
+	list := C.dataset_list_root()
+	for list != nil {
+		dataset := Dataset{
+			list:      list,
+			closeOnce: new(sync.Once),
+			Type:      DatasetType(C.dataset_type(list)),
+		}
+		dataset.Type = DatasetType(C.dataset_type(list))
 		err = dataset.ReloadProperties()
 		if err != nil {
 			return
 		}
 		datasets = append(datasets, dataset)
-		dataset.list = C.dataset_next(dataset.list)
+		list = C.dataset_next(list)
 	}
 	for ci := range datasets {
 		if err = datasets[ci].openChildren(); err != nil {
@@ -130,6 +136,7 @@ func DatasetOpenSingle(path string) (d Dataset, err error) {
 		err = fmt.Errorf("%s - %s", err.Error(), path)
 		return
 	}
+	d.closeOnce = new(sync.Once)
 	d.Type = DatasetType(C.dataset_type(d.list))
 	err = d.ReloadProperties()
 	if err != nil {
@@ -182,11 +189,13 @@ func DatasetCreate(path string, dtype DatasetType,
 // Close close dataset and all its recursive children datasets (close handle
 // and cleanup dataset object/s from memory)
 func (d *Dataset) Close() {
-	// path, _ := d.Path()
-	Global.Mtx.Lock()
-	C.dataset_list_close(d.list)
+	// if dataset was ever open
+	if d.closeOnce != nil {
+		d.closeOnce.Do(func() {
+			C.dataset_list_close(d.list)
+		})
+	}
 	d.list = nil
-	Global.Mtx.Unlock()
 	for _, cd := range d.Children {
 		cd.Close()
 	}
diff --git a/zfs_test.go b/zfs_test.go
index 99a9de8..d7f1d65 100644
--- a/zfs_test.go
+++ b/zfs_test.go
@@ -338,3 +338,35 @@ func ExampleDatasetOpenAll() {
 	}
 
 }
+
+func CopyAndDestroy(d *zfs.Dataset) (err error) {
+	if err = d.Destroy(false); err != nil {
+		return
+	}
+	d.Close()
+	return
+}
+
+func zfsTestDoubleFreeOnDestroy(t *testing.T) {
+	TSTDestroyPath := TSTPoolName + "/DESTROY"
+	println("TEST Doble Free On Destroy( ", TSTVolumePath, " ) ... ")
+	props := make(map[zfs.Prop]zfs.Property)
+	d, err := zfs.DatasetCreate(TSTDestroyPath, zfs.DatasetTypeFilesystem, props)
+	if err != nil {
+		t.Error(err)
+		return
+	}
+	d.Close()
+
+	d, err = zfs.DatasetOpen(TSTDestroyPath)
+	if err != nil {
+		t.Error(err)
+		return
+	}
+	defer d.Close()
+	if err = CopyAndDestroy(&d); err != nil {
+		t.Error(err)
+		return
+	}
+	print("PASS\n\n")
+}
diff --git a/zpool_test.go b/zpool_test.go
index 4f8d120..a56e34a 100644
--- a/zpool_test.go
+++ b/zpool_test.go
@@ -51,12 +51,18 @@ func createTestpoolVdisks() (err error) {
 	return
 }
 
+func removeVDisk(path string) {
+	if err := os.Remove(path); err != nil {
+		println("Error: ", err.Error())
+	}
+}
+
 // Cleanup sparse files used for tests
 func cleanupVDisks() {
 	// try cleanup
-	os.Remove(s1path)
-	os.Remove(s2path)
-	os.Remove(s3path)
+	removeVDisk(s1path)
+	removeVDisk(s2path)
+	removeVDisk(s3path)
 }
 
 /* ------------------------------------------------------------------------- */