use after free reported by ASAN

[cfergeau]

I ran example/linux/virtualization with ASAN enabled (sed -i '.bak' 's/FLAGS: /&-fsanitize=address /g' *.go), and the example program segfaulted with:

=================================
==53567==ERROR: AddressSanitizer: heap-use-after-free on address 0x60600002ae11 at pc 0x0000042115b5 bp 0x7ffeefbff7d0 sp 0x7ffeefbfef90
READ of size 37 at 0x60600002ae11 thread T0
    #0 0x42115b4 in wrap_strlen+0x184 (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x155b4)
    #1 0x7fff20691598 in _dispatch_strdup_if_mutable+0x11 (libdispatch.dylib:x86_64+0x2598)
    #2 0x7fff2069750c in _dispatch_lane_create_with_target+0x154 (libdispatch.dylib:x86_64+0x850c)
    #3 0x40adb5b in _cgo_a012ac8bb423_Cfunc_makeDispatchQueue+0x4b (virtualization:x86_64+0x40adb5b)
    #4 0x40635e3 in runtime.asmcgocall.abi0+0x63 (virtualization:x86_64+0x40635e3)

0x60600002ae11 is located 17 bytes inside of 64-byte region [0x60600002ae00,0x60600002ae40)
freed by thread T0 here:
    #0 0x4241096 in __sanitizer_mz_free+0x86 (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x45096)
    #1 0x7fff209ffade in _CFRelease+0x478 (CoreFoundation:x86_64h+0x14bade)
    #2 0x7fff206fa20e in AutoreleasePoolPage::releaseUntil(objc_object**)+0xa6 (libobjc.A.dylib:x86_64h+0x2620e)
    #3 0x7fff206dce2f in objc_autoreleasePoolPop+0xa0 (libobjc.A.dylib:x86_64h+0x8e2f)
    #4 0x40ac29e in _cgo_a012ac8bb423_Cfunc_getUUID+0xae (virtualization:x86_64+0x40ac29e)
    #5 0x40635e3 in runtime.asmcgocall.abi0+0x63 (virtualization:x86_64+0x40635e3)

previously allocated by thread T0 here:
    #0 0x4240dc2 in __sanitizer_mz_calloc+0x92 (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x44dc2)
    #1 0x7fff2067dff3 in _malloc_zone_calloc+0x3a (libsystem_malloc.dylib:x86_64+0x1bff3)
    #2 0x7fff208b7e61 in _CFRuntimeCreateInstance+0x125 (CoreFoundation:x86_64h+0x3e61)
    #3 0x7fff208b756b in __CFStringCreateImmutableFunnel3+0x76b (CoreFoundation:x86_64h+0x356b)
    #4 0x7fff208c3805 in CFStringCreateWithBytes+0x1a (CoreFoundation:x86_64h+0xf805)
    #5 0x7fff216890bd in +[NSString stringWithUTF8String:]+0x43 (Foundation:x86_64+0x250bd)
    #6 0x7fff21689ce6 in -[__NSConcreteUUID UUIDString]+0x40 (Foundation:x86_64+0x25ce6)
    #7 0x40ac26c in _cgo_a012ac8bb423_Cfunc_getUUID+0x7c (virtualization:x86_64+0x40ac26c)
    #8 0x40635e3 in runtime.asmcgocall.abi0+0x63 (virtualization:x86_64+0x40635e3)

SUMMARY: AddressSanitizer: heap-use-after-free (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x155b4) in wrap_strlen+0x184

SIGABRT: abort
PC=0x7fff2080d91e m=0 sigcode=0
signal arrived during cgo execution

goroutine 1 [syscall]:
runtime.cgocall(0x40adb10, 0xc000143d10)
	/usr/local/Cellar/go/1.18.4/libexec/src/runtime/cgocall.go:157 +0x5c fp=0xc000143ce8 sp=0xc000143cb0 pc=0x40085dc
github.com/Code-Hex/vz/v2._Cfunc_makeDispatchQueue(0x60600002ae11)
	_cgo_gotypes.go:386 +0x49 fp=0xc000143d10 sp=0xc000143ce8 pc=0x40a2189
github.com/Code-Hex/vz/v2.NewVirtualMachine(0x40fd2f0?)
	/Users/teuf/code/vz/virtualization.go:105 +0x45 fp=0xc000143d88 sp=0xc000143d10 pc=0x40a6a25
main.main()
	/Users/teuf/code/vz/example/linux/main.go:121 +0x626 fp=0xc000143f80 sp=0xc000143d88 pc=0x40a8306
runtime.main()
	/usr/local/Cellar/go/1.18.4/libexec/src/runtime/proc.go:250 +0x212 fp=0xc000143fe0 sp=0xc000143f80 pc=0x4038932
runtime.goexit()
	/usr/local/Cellar/go/1.18.4/libexec/src/runtime/asm_amd64.s:1571 +0x1 fp=0xc000143fe8 sp=0xc000143fe0 pc=0x4063901

I can workaround the problem with the change below, but I'm not familiar enough with go/objc memory handling in order to send a proper fix:

commit 2f29e7d6d904e45275f8ef5d3eaec4a7d07d291f
Author: Christophe Fergeau <cfergeau@redhat.com>
Date:   Sat Sep 10 13:24:56 2022 +0200

    hack: remove autoreleasepool to hack around use after free

diff --git a/objcutil.go b/objcutil.go
index e214be0..a93ad2b 100644
--- a/objcutil.go
+++ b/objcutil.go
@@ -111,10 +111,10 @@ void* getNSArrayItem(void *ptr, int i)
 const char *getUUID()
 {
 	const char *ret;
-	@autoreleasepool {
+	//@autoreleasepool {
 		NSString *uuid = [[NSUUID UUID] UUIDString];
 		ret = [uuid UTF8String];
-	}
+	//}
 	return ret;
 }
 */

[Code-Hex]

Thanks!
Exactly, We should not use release for NSUUID class https://developer.apple.com/documentation/foundation/nsuuid?language=objc

[cfergeau]

See #46 (comment) for a reproducer and a fix for this issue.

[cfergeau]

#52 has a test case to reproduce this
test: Add basic VirtualMachine unit test + test: Add 'test-asan' Makefile rule

It also has a fix for this: Fix use after free in getUUID()

[Code-Hex]

@cfergeau why didn't you send PR like this diff?

commit 2f29e7d6d904e45275f8ef5d3eaec4a7d07d291f
Author: Christophe Fergeau <cfergeau@redhat.com>
Date:   Sat Sep 10 13:24:56 2022 +0200

    hack: remove autoreleasepool to hack around use after free

diff --git a/objcutil.go b/objcutil.go
index e214be0..a93ad2b 100644
--- a/objcutil.go
+++ b/objcutil.go
@@ -111,10 +111,10 @@ void* getNSArrayItem(void *ptr, int i)
 const char *getUUID()
 {
 	const char *ret;
-	@autoreleasepool {
+	//@autoreleasepool {
 		NSString *uuid = [[NSUUID UUID] UUIDString];
 		ret = [uuid UTF8String];
-	}
+	//}
 	return ret;
 }
 */

[Code-Hex]

I agree with crc should use own fork repository 👍

[cfergeau]

@cfergeau why didn't you send PR like this diff?

I tried to make it slightly cleaner and sent https://github.com/Code-Hex/vz/pull/57/files which is about the same as this 'hack' commit. I can resend the same PR without the additional dependencies.

[Code-Hex]

@cfergeau No I said for only NSUUID.

This code is followed my comment. #47 (comment)

[cfergeau]

Oops, I'm mixing up issues ^^
I tried to address it with eae871e in order to keep the autoreleasepool as I was worried about memory leaks. If it's better to remove the autoreleasepool, I can make that change.

[Code-Hex]

@cfergeau As following my comment, NSUUID don't need to release. So autoreleasepool not necessary