[Libguestfs] [PATCH libnbd v2 2/9] golang: aio_buffer.go: Make it safer to use

[Nir Soffer]

If a Go program tries to use AioBuffer after calling AioBuffer.Free(),
the program may silently corrupt data, accessing memory that does not
belong to the buffer any more, or segfault if the address is not mapped.
In the worst case, it can corrupt memory silently. Calling Free() twice
may silently free unrelated memory.

Make the buffer safer to use by Freeing only on the first call and
setting the pointer to nil. This makes multiple calls to Free()
harmless, just like the underlying C.free().

Trying to access Bytes() and Get() after calling Free() will always
panic now, revealing the bug in the program.

Trying to use AioBuffer with libnbd API will likely segfault and panic.
I did not try to test this.

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 golang/aio_buffer.go                 |  5 +++-
 golang/libnbd_020_aio_buffer_test.go | 41 ++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go
index 2bc69a01..2b77d6ee 100644
--- a/golang/aio_buffer.go
+++ b/golang/aio_buffer.go
@@ -46,20 +46,23 @@ func MakeAioBuffer(size uint) AioBuffer {
 func FromBytes(buf []byte) AioBuffer {
 	size := len(buf)
 	ret := MakeAioBuffer(uint(size))
 	for i := 0; i < len(buf); i++ {
 		*ret.Get(uint(i)) = buf[i]
 	}
 	return ret
 }
 
 func (b *AioBuffer) Free() {
-	C.free(b.P)
+	if b.P != nil {
+		C.free(b.P)
+		b.P = nil
+	}
 }
 
 func (b *AioBuffer) Bytes() []byte {
 	return C.GoBytes(b.P, C.int(b.Size))
 }
 
 func (b *AioBuffer) Get(i uint) *byte {
 	return (*byte)(unsafe.Pointer(uintptr(b.P) + uintptr(i)))
 }
diff --git a/golang/libnbd_020_aio_buffer_test.go b/golang/libnbd_020_aio_buffer_test.go
index 5898746b..cec74ddc 100644
--- a/golang/libnbd_020_aio_buffer_test.go
+++ b/golang/libnbd_020_aio_buffer_test.go
@@ -53,20 +53,61 @@ func TestAioBuffer(t *testing.T) {
 
 	/* Create another buffer from Go slice. */
 	buf2 := FromBytes(zeroes)
 	defer buf2.Free()
 
 	if !bytes.Equal(buf2.Bytes(), zeroes) {
 		t.Fatalf("Expected %v, got %v", zeroes, buf2.Bytes())
 	}
 }
 
+func TestAioBufferFree(t *testing.T) {
+	buf := MakeAioBuffer(uint(32))
+
+	/* Free the underlying C array. */
+	buf.Free()
+
+	/* And clear the pointer. */
+	if buf.P != nil {
+		t.Fatal("Dangling pointer after Free()")
+	}
+
+	/* Additional Free does nothing. */
+	buf.Free()
+}
+
+func TestAioBufferBytesAfterFree(t *testing.T) {
+	buf := MakeAioBuffer(uint(32))
+	buf.Free()
+
+	defer func() {
+		if r := recover(); r == nil {
+			t.Fatal("Did not recover from panic calling Bytes() after Free()")
+		}
+	}()
+
+	buf.Bytes()
+}
+
+func TestAioBufferGetAfterFree(t *testing.T) {
+	buf := MakeAioBuffer(uint(32))
+	buf.Free()
+
+	defer func() {
+		if r := recover(); r == nil {
+			t.Fatal("Did not recover from panic calling Get() after Free()")
+		}
+	}()
+
+	*buf.Get(0) = 42
+}
+
 // Typical buffer size.
 const bufferSize uint = 256 * 1024
 
 // Benchmark creating an uninitialized buffer.
 func BenchmarkMakeAioBuffer(b *testing.B) {
 	for i := 0; i < b.N; i++ {
 		buf := MakeAioBuffer(bufferSize)
 		buf.Free()
 	}
 }
-- 
2.34.1