Merge pull request #124 from adriansr/fix-windows-crash (0.10.4)

Windows: fix a crash when splitting command-line arguments

CHANGELOG.md

@@ -12,6 +12,12 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Deprecated
 
+## [0.10.4]
+
+### Fixed
+
+- Fixed a crash when splitting command-line arguments under Windows. #124
+
 ## [0.10.3]
 
 ### Fixed

sys/windows/syscall_windows.go

@@ -503,9 +503,17 @@ func GetUserProcessParams(handle syscall.Handle, pbi ProcessBasicInformation) (p
 	return params, nil
 }
 
+// ReadProcessUnicodeString returns a zero-terminated UTF-16 string from another
+// process's memory.
 func ReadProcessUnicodeString(handle syscall.Handle, s *UnicodeString) ([]byte, error) {
-	buf := make([]byte, s.Size)
-	nRead, err := ReadProcessMemory(handle, s.Buffer, buf)
+	// Allocate an extra UTF-16 null character at the end in case the read string
+	// is not terminated.
+	extra := 2
+	if s.Size&1 != 0 {
+		extra = 3 // If size is odd, need 3 nulls to terminate.
+	}
+	buf := make([]byte, int(s.Size)+extra)
+	nRead, err := ReadProcessMemory(handle, s.Buffer, buf[:s.Size])
 	if err != nil {
 		return nil, err
 	}
@@ -515,12 +523,26 @@ func ReadProcessUnicodeString(handle syscall.Handle, s *UnicodeString) ([]byte,
 	return buf, nil
 }
 
-// Use Windows' CommandLineToArgv API to split an UTF-16 command line string
-// into a list of parameters.
+// ByteSliceToStringSlice uses CommandLineToArgv API to split an UTF-16 command
+// line string into a list of parameters.
 func ByteSliceToStringSlice(utf16 []byte) ([]string, error) {
-	if len(utf16) == 0 {
+	n := len(utf16)
+	// Discard odd byte
+	if n&1 != 0 {
+		n--
+		utf16 = utf16[:n]
+	}
+	if n == 0 {
 		return nil, nil
 	}
+	terminated := false
+	for i := 0; i < n && !terminated; i += 2 {
+		terminated = utf16[i] == 0 && utf16[i+1] == 0
+	}
+	if !terminated {
+		// Append a null uint16 at the end if terminator is missing
+		utf16 = append(utf16, 0, 0)
+	}
 	var numArgs int32
 	argsWide, err := syscall.CommandLineToArgv((*uint16)(unsafe.Pointer(&utf16[0])), &numArgs)
 	if err != nil {

sys/windows/syscall_windows_test.go

@@ -224,6 +224,7 @@ func TestReadProcessUnicodeString(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer syscall.CloseHandle(h)
 	info, err := NtQueryProcessBasicInformation(h)
 	if err != nil {
 		t.Fatal(err)
@@ -236,10 +237,36 @@ func TestReadProcessUnicodeString(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	defer syscall.CloseHandle(h)
 	assert.NoError(t, err)
 	assert.NotEmpty(t, read)
 }
+
+const currentProcessHandle = syscall.Handle(^uintptr(0))
+
+func TestReadProcessUnicodeStringTerminator(t *testing.T) {
+	data := []byte{'H', 0, 'E', 0, 'L', 0, 'L', 0, 'O', 0, 0, 0}
+	for n := len(data); n >= 0; n-- {
+		us := UnicodeString{
+			Buffer: uintptr(unsafe.Pointer(&data[0])),
+			Size:   uint16(n),
+		}
+		read, err := ReadProcessUnicodeString(currentProcessHandle, &us)
+		if err != nil {
+			t.Fatal(err)
+		}
+		nRead := len(read)
+		// Strings must match
+		assert.True(t, nRead >= n)
+		assert.Equal(t, data[:n], read[:n])
+		// result is an array of uint16, can't have odd length.
+		assert.True(t, nRead&1 == 0)
+		// Must include a zero terminator at the end.
+		assert.True(t, nRead >= 2)
+		assert.Zero(t, read[nRead-1])
+		assert.Zero(t, read[nRead-2])
+	}
+}
+
 func TestReadProcessUnicodeStringInvalidHandle(t *testing.T) {
 	var handle syscall.Handle
 	var cmd = UnicodeString{Size: 5, MaximumLength: 400, Buffer: 400}
@@ -264,6 +291,33 @@ func TestByteSliceToStringSliceEmptyBytes(t *testing.T) {
 	assert.Empty(t, cmd)
 }
 
+func mkUtf16bytes(s string) []byte {
+	n := len(s)
+	b := make([]byte, n*2)
+	for idx, val := range s {
+		*(*uint16)(unsafe.Pointer(&b[idx*2])) = uint16(val)
+	}
+	return b
+}
+
+func TestByteSliceToStringSliceNotTerminated(t *testing.T) {
+	b := mkUtf16bytes("Hello World")
+	cmd, err := ByteSliceToStringSlice(b)
+	assert.NoError(t, err)
+	assert.Len(t, cmd, 2)
+	assert.Equal(t, "Hello", cmd[0])
+	assert.Equal(t, "World", cmd[1])
+}
+
+func TestByteSliceToStringSliceNotOddSize(t *testing.T) {
+	b := mkUtf16bytes("BAD")[:5]
+	cmd, err := ByteSliceToStringSlice(b)
+	assert.NoError(t, err)
+	assert.Len(t, cmd, 1)
+	// Odd character is dropped
+	assert.Equal(t, "BA", cmd[0])
+}
+
 func TestReadProcessMemory(t *testing.T) {
 	h, err := syscall.OpenProcess(syscall.PROCESS_QUERY_INFORMATION|PROCESS_VM_READ, false, uint32(syscall.Getpid()))
 	if err != nil {