From 6442b6ca13dd00f0bec40c52863a19d6c07f83b8 Mon Sep 17 00:00:00 2001 From: Kevin Parsons Date: Mon, 26 Apr 2021 15:08:11 -0700 Subject: [PATCH 1/4] Fix syscall error handling in vhd and pkg/security These packages had incorrect error handling for their generated syscall bindings. The functions they were calling returned errors directly, but the binding was written such that the generated code was calling GetLastError instead. Thankfully, this did not affect the detection of whether or not an error had occurred, it only caused the value returned in the case of an error to be not the right error code. Signed-off-by: Kevin Parsons (cherry picked from commit 9f0ab2cc0a345ed3a0f844cf756a3f01561ec94f) Signed-off-by: Kevin Parsons --- pkg/security/syscall_windows.go | 6 ++-- pkg/security/zsyscall_windows.go | 24 +++++++-------- vhd/vhd.go | 10 +++--- vhd/zvhd_windows.go | 52 ++++++++++++++++---------------- 4 files changed, 46 insertions(+), 46 deletions(-) diff --git a/pkg/security/syscall_windows.go b/pkg/security/syscall_windows.go index c40c2739..d7096716 100644 --- a/pkg/security/syscall_windows.go +++ b/pkg/security/syscall_windows.go @@ -2,6 +2,6 @@ package security //go:generate go run mksyscall_windows.go -output zsyscall_windows.go syscall_windows.go -//sys getSecurityInfo(handle syscall.Handle, objectType uint32, si uint32, ppsidOwner **uintptr, ppsidGroup **uintptr, ppDacl *uintptr, ppSacl *uintptr, ppSecurityDescriptor *uintptr) (err error) [failretval!=0] = advapi32.GetSecurityInfo -//sys setSecurityInfo(handle syscall.Handle, objectType uint32, si uint32, psidOwner uintptr, psidGroup uintptr, pDacl uintptr, pSacl uintptr) (err error) [failretval!=0] = advapi32.SetSecurityInfo -//sys setEntriesInAcl(count uintptr, pListOfEEs uintptr, oldAcl uintptr, newAcl *uintptr) (err error) [failretval!=0] = advapi32.SetEntriesInAclW +//sys getSecurityInfo(handle syscall.Handle, objectType uint32, si uint32, ppsidOwner **uintptr, ppsidGroup **uintptr, ppDacl *uintptr, ppSacl *uintptr, ppSecurityDescriptor *uintptr) (win32err error) = advapi32.GetSecurityInfo +//sys setSecurityInfo(handle syscall.Handle, objectType uint32, si uint32, psidOwner uintptr, psidGroup uintptr, pDacl uintptr, pSacl uintptr) (win32err error) = advapi32.SetSecurityInfo +//sys setEntriesInAcl(count uintptr, pListOfEEs uintptr, oldAcl uintptr, newAcl *uintptr) (win32err error) = advapi32.SetEntriesInAclW diff --git a/pkg/security/zsyscall_windows.go b/pkg/security/zsyscall_windows.go index 4a90cb3c..4084680e 100644 --- a/pkg/security/zsyscall_windows.go +++ b/pkg/security/zsyscall_windows.go @@ -45,26 +45,26 @@ var ( procSetSecurityInfo = modadvapi32.NewProc("SetSecurityInfo") ) -func getSecurityInfo(handle syscall.Handle, objectType uint32, si uint32, ppsidOwner **uintptr, ppsidGroup **uintptr, ppDacl *uintptr, ppSacl *uintptr, ppSecurityDescriptor *uintptr) (err error) { - r1, _, e1 := syscall.Syscall9(procGetSecurityInfo.Addr(), 8, uintptr(handle), uintptr(objectType), uintptr(si), uintptr(unsafe.Pointer(ppsidOwner)), uintptr(unsafe.Pointer(ppsidGroup)), uintptr(unsafe.Pointer(ppDacl)), uintptr(unsafe.Pointer(ppSacl)), uintptr(unsafe.Pointer(ppSecurityDescriptor)), 0) - if r1 != 0 { - err = errnoErr(e1) +func getSecurityInfo(handle syscall.Handle, objectType uint32, si uint32, ppsidOwner **uintptr, ppsidGroup **uintptr, ppDacl *uintptr, ppSacl *uintptr, ppSecurityDescriptor *uintptr) (win32err error) { + r0, _, _ := syscall.Syscall9(procGetSecurityInfo.Addr(), 8, uintptr(handle), uintptr(objectType), uintptr(si), uintptr(unsafe.Pointer(ppsidOwner)), uintptr(unsafe.Pointer(ppsidGroup)), uintptr(unsafe.Pointer(ppDacl)), uintptr(unsafe.Pointer(ppSacl)), uintptr(unsafe.Pointer(ppSecurityDescriptor)), 0) + if r0 != 0 { + win32err = syscall.Errno(r0) } return } -func setEntriesInAcl(count uintptr, pListOfEEs uintptr, oldAcl uintptr, newAcl *uintptr) (err error) { - r1, _, e1 := syscall.Syscall6(procSetEntriesInAclW.Addr(), 4, uintptr(count), uintptr(pListOfEEs), uintptr(oldAcl), uintptr(unsafe.Pointer(newAcl)), 0, 0) - if r1 != 0 { - err = errnoErr(e1) +func setEntriesInAcl(count uintptr, pListOfEEs uintptr, oldAcl uintptr, newAcl *uintptr) (win32err error) { + r0, _, _ := syscall.Syscall6(procSetEntriesInAclW.Addr(), 4, uintptr(count), uintptr(pListOfEEs), uintptr(oldAcl), uintptr(unsafe.Pointer(newAcl)), 0, 0) + if r0 != 0 { + win32err = syscall.Errno(r0) } return } -func setSecurityInfo(handle syscall.Handle, objectType uint32, si uint32, psidOwner uintptr, psidGroup uintptr, pDacl uintptr, pSacl uintptr) (err error) { - r1, _, e1 := syscall.Syscall9(procSetSecurityInfo.Addr(), 7, uintptr(handle), uintptr(objectType), uintptr(si), uintptr(psidOwner), uintptr(psidGroup), uintptr(pDacl), uintptr(pSacl), 0, 0) - if r1 != 0 { - err = errnoErr(e1) +func setSecurityInfo(handle syscall.Handle, objectType uint32, si uint32, psidOwner uintptr, psidGroup uintptr, pDacl uintptr, pSacl uintptr) (win32err error) { + r0, _, _ := syscall.Syscall9(procSetSecurityInfo.Addr(), 7, uintptr(handle), uintptr(objectType), uintptr(si), uintptr(psidOwner), uintptr(psidGroup), uintptr(pDacl), uintptr(pSacl), 0, 0) + if r0 != 0 { + win32err = syscall.Errno(r0) } return } diff --git a/vhd/vhd.go b/vhd/vhd.go index b03b789e..a33a36c0 100644 --- a/vhd/vhd.go +++ b/vhd/vhd.go @@ -13,11 +13,11 @@ import ( //go:generate go run mksyscall_windows.go -output zvhd_windows.go vhd.go -//sys createVirtualDisk(virtualStorageType *VirtualStorageType, path string, virtualDiskAccessMask uint32, securityDescriptor *uintptr, createVirtualDiskFlags uint32, providerSpecificFlags uint32, parameters *CreateVirtualDiskParameters, overlapped *syscall.Overlapped, handle *syscall.Handle) (err error) [failretval != 0] = virtdisk.CreateVirtualDisk -//sys openVirtualDisk(virtualStorageType *VirtualStorageType, path string, virtualDiskAccessMask uint32, openVirtualDiskFlags uint32, parameters *OpenVirtualDiskParameters, handle *syscall.Handle) (err error) [failretval != 0] = virtdisk.OpenVirtualDisk -//sys attachVirtualDisk(handle syscall.Handle, securityDescriptor *uintptr, attachVirtualDiskFlag uint32, providerSpecificFlags uint32, parameters *AttachVirtualDiskParameters, overlapped *syscall.Overlapped) (err error) [failretval != 0] = virtdisk.AttachVirtualDisk -//sys detachVirtualDisk(handle syscall.Handle, detachVirtualDiskFlags uint32, providerSpecificFlags uint32) (err error) [failretval != 0] = virtdisk.DetachVirtualDisk -//sys getVirtualDiskPhysicalPath(handle syscall.Handle, diskPathSizeInBytes *uint32, buffer *uint16) (err error) [failretval != 0] = virtdisk.GetVirtualDiskPhysicalPath +//sys createVirtualDisk(virtualStorageType *VirtualStorageType, path string, virtualDiskAccessMask uint32, securityDescriptor *uintptr, createVirtualDiskFlags uint32, providerSpecificFlags uint32, parameters *CreateVirtualDiskParameters, overlapped *syscall.Overlapped, handle *syscall.Handle) (win32err error) = virtdisk.CreateVirtualDisk +//sys openVirtualDisk(virtualStorageType *VirtualStorageType, path string, virtualDiskAccessMask uint32, openVirtualDiskFlags uint32, parameters *OpenVirtualDiskParameters, handle *syscall.Handle) (win32err error) = virtdisk.OpenVirtualDisk +//sys attachVirtualDisk(handle syscall.Handle, securityDescriptor *uintptr, attachVirtualDiskFlag uint32, providerSpecificFlags uint32, parameters *AttachVirtualDiskParameters, overlapped *syscall.Overlapped) (win32err error) = virtdisk.AttachVirtualDisk +//sys detachVirtualDisk(handle syscall.Handle, detachVirtualDiskFlags uint32, providerSpecificFlags uint32) (win32err error) = virtdisk.DetachVirtualDisk +//sys getVirtualDiskPhysicalPath(handle syscall.Handle, diskPathSizeInBytes *uint32, buffer *uint16) (win32err error) = virtdisk.GetVirtualDiskPhysicalPath type ( CreateVirtualDiskFlag uint32 diff --git a/vhd/zvhd_windows.go b/vhd/zvhd_windows.go index 572f7b42..7fb5f365 100644 --- a/vhd/zvhd_windows.go +++ b/vhd/zvhd_windows.go @@ -47,60 +47,60 @@ var ( procOpenVirtualDisk = modvirtdisk.NewProc("OpenVirtualDisk") ) -func attachVirtualDisk(handle syscall.Handle, securityDescriptor *uintptr, attachVirtualDiskFlag uint32, providerSpecificFlags uint32, parameters *AttachVirtualDiskParameters, overlapped *syscall.Overlapped) (err error) { - r1, _, e1 := syscall.Syscall6(procAttachVirtualDisk.Addr(), 6, uintptr(handle), uintptr(unsafe.Pointer(securityDescriptor)), uintptr(attachVirtualDiskFlag), uintptr(providerSpecificFlags), uintptr(unsafe.Pointer(parameters)), uintptr(unsafe.Pointer(overlapped))) - if r1 != 0 { - err = errnoErr(e1) +func attachVirtualDisk(handle syscall.Handle, securityDescriptor *uintptr, attachVirtualDiskFlag uint32, providerSpecificFlags uint32, parameters *AttachVirtualDiskParameters, overlapped *syscall.Overlapped) (win32err error) { + r0, _, _ := syscall.Syscall6(procAttachVirtualDisk.Addr(), 6, uintptr(handle), uintptr(unsafe.Pointer(securityDescriptor)), uintptr(attachVirtualDiskFlag), uintptr(providerSpecificFlags), uintptr(unsafe.Pointer(parameters)), uintptr(unsafe.Pointer(overlapped))) + if r0 != 0 { + win32err = syscall.Errno(r0) } return } -func createVirtualDisk(virtualStorageType *VirtualStorageType, path string, virtualDiskAccessMask uint32, securityDescriptor *uintptr, createVirtualDiskFlags uint32, providerSpecificFlags uint32, parameters *CreateVirtualDiskParameters, overlapped *syscall.Overlapped, handle *syscall.Handle) (err error) { +func createVirtualDisk(virtualStorageType *VirtualStorageType, path string, virtualDiskAccessMask uint32, securityDescriptor *uintptr, createVirtualDiskFlags uint32, providerSpecificFlags uint32, parameters *CreateVirtualDiskParameters, overlapped *syscall.Overlapped, handle *syscall.Handle) (win32err error) { var _p0 *uint16 - _p0, err = syscall.UTF16PtrFromString(path) - if err != nil { + _p0, win32err = syscall.UTF16PtrFromString(path) + if win32err != nil { return } return _createVirtualDisk(virtualStorageType, _p0, virtualDiskAccessMask, securityDescriptor, createVirtualDiskFlags, providerSpecificFlags, parameters, overlapped, handle) } -func _createVirtualDisk(virtualStorageType *VirtualStorageType, path *uint16, virtualDiskAccessMask uint32, securityDescriptor *uintptr, createVirtualDiskFlags uint32, providerSpecificFlags uint32, parameters *CreateVirtualDiskParameters, overlapped *syscall.Overlapped, handle *syscall.Handle) (err error) { - r1, _, e1 := syscall.Syscall9(procCreateVirtualDisk.Addr(), 9, uintptr(unsafe.Pointer(virtualStorageType)), uintptr(unsafe.Pointer(path)), uintptr(virtualDiskAccessMask), uintptr(unsafe.Pointer(securityDescriptor)), uintptr(createVirtualDiskFlags), uintptr(providerSpecificFlags), uintptr(unsafe.Pointer(parameters)), uintptr(unsafe.Pointer(overlapped)), uintptr(unsafe.Pointer(handle))) - if r1 != 0 { - err = errnoErr(e1) +func _createVirtualDisk(virtualStorageType *VirtualStorageType, path *uint16, virtualDiskAccessMask uint32, securityDescriptor *uintptr, createVirtualDiskFlags uint32, providerSpecificFlags uint32, parameters *CreateVirtualDiskParameters, overlapped *syscall.Overlapped, handle *syscall.Handle) (win32err error) { + r0, _, _ := syscall.Syscall9(procCreateVirtualDisk.Addr(), 9, uintptr(unsafe.Pointer(virtualStorageType)), uintptr(unsafe.Pointer(path)), uintptr(virtualDiskAccessMask), uintptr(unsafe.Pointer(securityDescriptor)), uintptr(createVirtualDiskFlags), uintptr(providerSpecificFlags), uintptr(unsafe.Pointer(parameters)), uintptr(unsafe.Pointer(overlapped)), uintptr(unsafe.Pointer(handle))) + if r0 != 0 { + win32err = syscall.Errno(r0) } return } -func detachVirtualDisk(handle syscall.Handle, detachVirtualDiskFlags uint32, providerSpecificFlags uint32) (err error) { - r1, _, e1 := syscall.Syscall(procDetachVirtualDisk.Addr(), 3, uintptr(handle), uintptr(detachVirtualDiskFlags), uintptr(providerSpecificFlags)) - if r1 != 0 { - err = errnoErr(e1) +func detachVirtualDisk(handle syscall.Handle, detachVirtualDiskFlags uint32, providerSpecificFlags uint32) (win32err error) { + r0, _, _ := syscall.Syscall(procDetachVirtualDisk.Addr(), 3, uintptr(handle), uintptr(detachVirtualDiskFlags), uintptr(providerSpecificFlags)) + if r0 != 0 { + win32err = syscall.Errno(r0) } return } -func getVirtualDiskPhysicalPath(handle syscall.Handle, diskPathSizeInBytes *uint32, buffer *uint16) (err error) { - r1, _, e1 := syscall.Syscall(procGetVirtualDiskPhysicalPath.Addr(), 3, uintptr(handle), uintptr(unsafe.Pointer(diskPathSizeInBytes)), uintptr(unsafe.Pointer(buffer))) - if r1 != 0 { - err = errnoErr(e1) +func getVirtualDiskPhysicalPath(handle syscall.Handle, diskPathSizeInBytes *uint32, buffer *uint16) (win32err error) { + r0, _, _ := syscall.Syscall(procGetVirtualDiskPhysicalPath.Addr(), 3, uintptr(handle), uintptr(unsafe.Pointer(diskPathSizeInBytes)), uintptr(unsafe.Pointer(buffer))) + if r0 != 0 { + win32err = syscall.Errno(r0) } return } -func openVirtualDisk(virtualStorageType *VirtualStorageType, path string, virtualDiskAccessMask uint32, openVirtualDiskFlags uint32, parameters *OpenVirtualDiskParameters, handle *syscall.Handle) (err error) { +func openVirtualDisk(virtualStorageType *VirtualStorageType, path string, virtualDiskAccessMask uint32, openVirtualDiskFlags uint32, parameters *OpenVirtualDiskParameters, handle *syscall.Handle) (win32err error) { var _p0 *uint16 - _p0, err = syscall.UTF16PtrFromString(path) - if err != nil { + _p0, win32err = syscall.UTF16PtrFromString(path) + if win32err != nil { return } return _openVirtualDisk(virtualStorageType, _p0, virtualDiskAccessMask, openVirtualDiskFlags, parameters, handle) } -func _openVirtualDisk(virtualStorageType *VirtualStorageType, path *uint16, virtualDiskAccessMask uint32, openVirtualDiskFlags uint32, parameters *OpenVirtualDiskParameters, handle *syscall.Handle) (err error) { - r1, _, e1 := syscall.Syscall6(procOpenVirtualDisk.Addr(), 6, uintptr(unsafe.Pointer(virtualStorageType)), uintptr(unsafe.Pointer(path)), uintptr(virtualDiskAccessMask), uintptr(openVirtualDiskFlags), uintptr(unsafe.Pointer(parameters)), uintptr(unsafe.Pointer(handle))) - if r1 != 0 { - err = errnoErr(e1) +func _openVirtualDisk(virtualStorageType *VirtualStorageType, path *uint16, virtualDiskAccessMask uint32, openVirtualDiskFlags uint32, parameters *OpenVirtualDiskParameters, handle *syscall.Handle) (win32err error) { + r0, _, _ := syscall.Syscall6(procOpenVirtualDisk.Addr(), 6, uintptr(unsafe.Pointer(virtualStorageType)), uintptr(unsafe.Pointer(path)), uintptr(virtualDiskAccessMask), uintptr(openVirtualDiskFlags), uintptr(unsafe.Pointer(parameters)), uintptr(unsafe.Pointer(handle))) + if r0 != 0 { + win32err = syscall.Errno(r0) } return } From 38765a606aff2022e1392ed02f24ab430762d30b Mon Sep 17 00:00:00 2001 From: Kyle W Date: Mon, 14 Jun 2021 13:44:35 -0700 Subject: [PATCH 2/4] Fix corruption in x86 callback (cherry picked from commit 0b148d19cf647cc36cf12569c01f1721ad6f66f1) Signed-off-by: Kevin Parsons --- pkg/etw/provider.go | 9 --------- pkg/etw/wrapper_32.go | 15 +++++++++++++++ pkg/etw/wrapper_64.go | 10 ++++++++++ 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/pkg/etw/provider.go b/pkg/etw/provider.go index e912b51b..a5b90d03 100644 --- a/pkg/etw/provider.go +++ b/pkg/etw/provider.go @@ -83,15 +83,6 @@ func providerCallback(sourceID guid.GUID, state ProviderState, level Level, matc } } -// providerCallbackAdapter acts as the first-level callback from the C/ETW side -// for provider notifications. Because Go has trouble with callback arguments of -// different size, it has only pointer-sized arguments, which are then cast to -// the appropriate types when calling providerCallback. -func providerCallbackAdapter(sourceID *guid.GUID, state uintptr, level uintptr, matchAnyKeyword uintptr, matchAllKeyword uintptr, filterData uintptr, i uintptr) uintptr { - providerCallback(*sourceID, ProviderState(state), Level(level), uint64(matchAnyKeyword), uint64(matchAllKeyword), filterData, i) - return 0 -} - // providerIDFromName generates a provider ID based on the provider name. It // uses the same algorithm as used by .NET's EventSource class, which is based // on RFC 4122. More information on the algorithm can be found here: diff --git a/pkg/etw/wrapper_32.go b/pkg/etw/wrapper_32.go index 5766d4d7..9b66cee6 100644 --- a/pkg/etw/wrapper_32.go +++ b/pkg/etw/wrapper_32.go @@ -4,6 +4,7 @@ package etw import ( + "github.com/Microsoft/go-winio/pkg/guid" "golang.org/x/sys/windows" ) @@ -50,3 +51,17 @@ func eventSetInformation( information, length) } + +// providerCallbackAdapter acts as the first-level callback from the C/ETW side +// for provider notifications. Because Go has trouble with callback arguments of +// different size, it has only pointer-sized arguments, which are then cast to +// the appropriate types when calling providerCallback. +// For x86, the matchAny and matchAll keywords need to be assembled from two +// 32-bit integers, because the max size of an argument is uintptr, but those +// two arguments are actually 64-bit integers. +func providerCallbackAdapter(sourceID *guid.GUID, state uint32, level uint8, matchAnyKeyword_low uint32, matchAnyKeyword_high uint32, matchAllKeyword_low uint32, matchAllKeyword_high uint32, filterData uintptr, i uintptr) uintptr { + matchAnyKeyword := uint64(matchAnyKeyword_high) << 32 | uint64(matchAnyKeyword_low) + matchAllKeyword := uint64(matchAllKeyword_high) << 32 | uint64(matchAllKeyword_low) + providerCallback(*sourceID, ProviderState(state), Level(level), uint64(matchAnyKeyword), uint64(matchAllKeyword), filterData, i) + return 0 +} \ No newline at end of file diff --git a/pkg/etw/wrapper_64.go b/pkg/etw/wrapper_64.go index c78d8d2b..1969ade0 100644 --- a/pkg/etw/wrapper_64.go +++ b/pkg/etw/wrapper_64.go @@ -4,6 +4,7 @@ package etw import ( + "github.com/Microsoft/go-winio/pkg/guid" "golang.org/x/sys/windows" ) @@ -40,3 +41,12 @@ func eventSetInformation( information, length) } + +// providerCallbackAdapter acts as the first-level callback from the C/ETW side +// for provider notifications. Because Go has trouble with callback arguments of +// different size, it has only pointer-sized arguments, which are then cast to +// the appropriate types when calling providerCallback. +func providerCallbackAdapter(sourceID *guid.GUID, state uint32, level uint8, matchAnyKeyword uintptr, matchAllKeyword uintptr, filterData uintptr, i uintptr) uintptr { + providerCallback(*sourceID, ProviderState(state), Level(level), uint64(matchAnyKeyword), uint64(matchAllKeyword), filterData, i) + return 0 +} \ No newline at end of file From 7329d86b85f45459f3d897c38cb827618107f2f2 Mon Sep 17 00:00:00 2001 From: Kevin Parsons Date: Thu, 16 Sep 2021 17:23:13 -0700 Subject: [PATCH 3/4] backuptar: Fix sparse file handling A recent OS change altered how sparse files are represented in backup streams. This caused backuptar to no longer work with certain files. The specific behavior that changed is as follows: - Empty sparse files (size = 0), previously did not have any data or sparse block streams in the backup stream. Now, they will have a data stream with size = 0, and no sparse block streams. - Sparse files with a single allocated range (e.g. a normal file that has the sparse attribute set) previously would not show as sparse in the backup stream. Now, they will show as sparse. The old backuptar behavior assumed that if the sparse flag was set on the data stream, then there would always be a set of sparse blocks following. These changes break this assumption, and so require special handling. It is unsupported to have a data stream, marked sparse, that contains file content AND a series of sparse block streams following. As far as I can tell this is not a valid case for backup streams. This change also cleans up some code and error messages, and expands on the test coverage for backuptar. For more information on backup stream format see: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-bkup/f67950c8-d583-469a-83dd-c4ff4cedf533 Signed-off-by: Kevin Parsons (cherry picked from commit 33a480106a69cee4c91265bff814a01cc27458a5) Signed-off-by: Kevin Parsons --- backuptar/tar.go | 74 +++++++++----- backuptar/tar_test.go | 232 +++++++++++++++++++++++++++++++++--------- 2 files changed, 234 insertions(+), 72 deletions(-) diff --git a/backuptar/tar.go b/backuptar/tar.go index 088a43c6..b32bee3c 100644 --- a/backuptar/tar.go +++ b/backuptar/tar.go @@ -5,7 +5,6 @@ package backuptar import ( "archive/tar" "encoding/base64" - "errors" "fmt" "io" "io/ioutil" @@ -41,19 +40,14 @@ const ( hdrCreationTime = "LIBARCHIVE.creationtime" ) -func writeZeroes(w io.Writer, count int64) error { - buf := make([]byte, 8192) - c := len(buf) - for i := int64(0); i < count; i += int64(c) { - if int64(c) > count-i { - c = int(count - i) - } - _, err := w.Write(buf[:c]) - if err != nil { - return err - } +// zeroReader is an io.Reader that always returns 0s. +type zeroReader struct{} + +func (zr zeroReader) Read(b []byte) (int, error) { + for i := range b { + b[i] = 0 } - return nil + return len(b), nil } func copySparse(t *tar.Writer, br *winio.BackupStreamReader) error { @@ -70,16 +64,26 @@ func copySparse(t *tar.Writer, br *winio.BackupStreamReader) error { return fmt.Errorf("unexpected stream %d", bhdr.Id) } + // We can't seek backwards, since we have already written that data to the tar.Writer. + if bhdr.Offset < curOffset { + return fmt.Errorf("cannot seek back from %d to %d", curOffset, bhdr.Offset) + } // archive/tar does not support writing sparse files // so just write zeroes to catch up to the current offset. - err = writeZeroes(t, bhdr.Offset-curOffset) + if _, err := io.CopyN(t, zeroReader{}, bhdr.Offset-curOffset); err != nil { + return fmt.Errorf("seek to offset %d: %s", bhdr.Offset, err) + } if bhdr.Size == 0 { + // A sparse block with size = 0 is used to mark the end of the sparse blocks. break } n, err := io.Copy(t, br) if err != nil { return err } + if n != bhdr.Size { + return fmt.Errorf("copied %d bytes instead of %d at offset %d", n, bhdr.Size, bhdr.Offset) + } curOffset = bhdr.Offset + n } return nil @@ -220,20 +224,44 @@ func WriteTarFileFromBackupStream(t *tar.Writer, r io.Reader, name string, size } } + // The logic for copying file contents is fairly complicated due to the need for handling sparse files, + // and the weird ways they are represented by BackupRead. A normal file will always either have a data stream + // with size and content, or no data stream at all (if empty). However, for a sparse file, the content can also + // be represented using a series of sparse block streams following the data stream. Additionally, the way sparse + // files are handled by BackupRead has changed in the OS recently. The specifics of the representation are described + // in the list at the bottom of this block comment. + // + // Sparse files can be represented in four different ways, based on the specifics of the file. + // - Size = 0: + // Previously: BackupRead yields no data stream and no sparse block streams. + // Recently: BackupRead yields a data stream with size = 0. There are no following sparse block streams. + // - Size > 0, no allocated ranges: + // BackupRead yields a data stream with size = 0. Following is a single sparse block stream with + // size = 0 and offset = . + // - Size > 0, one allocated range: + // BackupRead yields a data stream with size = containing the file contents. There are no + // sparse block streams. This is the case if you take a normal file with contents and simply set the + // sparse flag on it. + // - Size > 0, multiple allocated ranges: + // BackupRead yields a data stream with size = 0. Following are sparse block streams for each allocated + // range of the file containing the range contents. Finally there is a sparse block stream with + // size = 0 and offset = . + if dataHdr != nil { // A data stream was found. Copy the data. - if (dataHdr.Attributes & winio.StreamSparseAttributes) == 0 { + // We assume that we will either have a data stream size > 0 XOR have sparse block streams. + if dataHdr.Size > 0 || (dataHdr.Attributes&winio.StreamSparseAttributes) == 0 { if size != dataHdr.Size { return fmt.Errorf("%s: mismatch between file size %d and header size %d", name, size, dataHdr.Size) } - _, err = io.Copy(t, br) - if err != nil { - return err + if _, err = io.Copy(t, br); err != nil { + return fmt.Errorf("%s: copying contents from data stream: %s", name, err) } - } else { - err = copySparse(t, br) - if err != nil { - return err + } else if size > 0 { + // As of a recent OS change, BackupRead now returns a data stream for empty sparse files. + // These files have no sparse block streams, so skip the copySparse call if file size = 0. + if err = copySparse(t, br); err != nil { + return fmt.Errorf("%s: copying contents from sparse block stream: %s", name, err) } } } @@ -278,7 +306,7 @@ func WriteTarFileFromBackupStream(t *tar.Writer, r io.Reader, name string, size } else { // Unsupported for now, since the size of the alternate stream is not present // in the backup stream until after the data has been read. - return errors.New("tar of sparse alternate data streams is unsupported") + return fmt.Errorf("%s: tar of sparse alternate data streams is unsupported", name) } case winio.BackupEaData, winio.BackupLink, winio.BackupPropertyData, winio.BackupObjectId, winio.BackupTxfsData: // ignore these streams diff --git a/backuptar/tar_test.go b/backuptar/tar_test.go index 8be0bf34..cc2cbf09 100644 --- a/backuptar/tar_test.go +++ b/backuptar/tar_test.go @@ -5,6 +5,7 @@ package backuptar import ( "archive/tar" "bytes" + "io" "io/ioutil" "os" "path/filepath" @@ -12,6 +13,7 @@ import ( "testing" "github.com/Microsoft/go-winio" + "golang.org/x/sys/windows" ) func ensurePresent(t *testing.T, m map[string]string, keys ...string) { @@ -22,65 +24,197 @@ func ensurePresent(t *testing.T, m map[string]string, keys ...string) { } } -func TestRoundTrip(t *testing.T) { - f, err := ioutil.TempFile("", "tst") - if err != nil { - t.Fatal(err) - } - defer f.Close() - defer os.Remove(f.Name()) - - if _, err = f.Write([]byte("testing 1 2 3\n")); err != nil { - t.Fatal(err) - } - - if _, err = f.Seek(0, 0); err != nil { +func setSparse(t *testing.T, f *os.File) { + const FSCTL_SET_SPARSE uint32 = 0x900c4 + if err := windows.DeviceIoControl(windows.Handle(f.Fd()), FSCTL_SET_SPARSE, nil, 0, nil, 0, nil, nil); err != nil { t.Fatal(err) } +} - fi, err := f.Stat() - if err != nil { - t.Fatal(err) +// compareReaders validates that two readers contain the exact same data. +func compareReaders(t *testing.T, rActual io.Reader, rExpected io.Reader) { + const size = 8 * 1024 + var bufExpected, bufActual [size]byte + var readCount int64 + // Loop, first reading from rExpected, then reading the same amount from rActual. + // For each set of reads, compare the bytes to make sure they are identical. + // When we run out of data in rExpected, exit the loop. + for { + // Do a read from rExpected and see how many bytes we get. + nExpected, err := rExpected.Read(bufExpected[:]) + if err == io.EOF && nExpected == 0 { + break + } else if err != nil && err != io.EOF { + t.Fatalf("Failed reading from rExpected at %d: %s", readCount, err) + } + // Do a ReadFull from rActual for the same number of bytes we got from rExpected. + if nActual, err := io.ReadFull(rActual, bufActual[:nExpected]); err != nil { + t.Fatalf("Only read %d bytes out of %d from rActual at %d: %s", nActual, nExpected, readCount, err) + } + readCount += int64(nExpected) + for i, bExpected := range bufExpected[:nExpected] { + if bExpected != bufActual[i] { + t.Fatalf("Mismatched bytes at %d. got 0x%x, expected 0x%x", i, bufActual[i], bExpected) + } + } } - - bi, err := winio.GetFileBasicInfo(f) - if err != nil { - t.Fatal(err) - } - - br := winio.NewBackupFileReader(f, true) - defer br.Close() - - var buf bytes.Buffer - tw := tar.NewWriter(&buf) - - err = WriteTarFileFromBackupStream(tw, br, f.Name(), fi.Size(), bi) - if err != nil { - t.Fatal(err) + // Now we just need to make sure there isn't any further data in rActual. + var b [1]byte + if n, err := rActual.Read(b[:]); n != 0 || err != io.EOF { + t.Fatalf("rActual didn't return EOF at expected end. Read %d bytes with error %s", n, err) } +} - tr := tar.NewReader(&buf) - hdr, err := tr.Next() - if err != nil { - t.Fatal(err) +func TestRoundTrip(t *testing.T) { + // Each test case is a name mapped to a function which must create a file and return its path. + // The test then round-trips that file through backuptar, and validates the output matches the input. + for name, setup := range map[string]func(*testing.T) string{ + "normalFile": func(t *testing.T) string { + path := filepath.Join(t.TempDir(), "foo.txt") + if err := ioutil.WriteFile(path, []byte("testing 1 2 3\n"), 0644); err != nil { + t.Fatal(err) + } + return path + }, + "normalFileEmpty": func(t *testing.T) string { + path := filepath.Join(t.TempDir(), "foo.txt") + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0644) + if err != nil { + t.Fatal(err) + } + defer f.Close() + return path + }, + "sparseFileEmpty": func(t *testing.T) string { + path := filepath.Join(t.TempDir(), "foo.txt") + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0644) + if err != nil { + t.Fatal(err) + } + defer f.Close() + setSparse(t, f) + return path + }, + "sparseFileWithNoAllocatedRanges": func(t *testing.T) string { + path := filepath.Join(t.TempDir(), "foo.txt") + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0644) + if err != nil { + t.Fatal(err) + } + defer f.Close() + setSparse(t, f) + // Set file size without writing data to produce a file with size > 0 + // but no allocated ranges. + if err := f.Truncate(1000000); err != nil { + t.Fatal(err) + } + return path + }, + "sparseFileWithOneAllocatedRange": func(t *testing.T) string { + path := filepath.Join(t.TempDir(), "foo.txt") + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0644) + if err != nil { + t.Fatal(err) + } + defer f.Close() + setSparse(t, f) + if _, err := f.WriteString("test sparse data"); err != nil { + t.Fatal(err) + } + return path + }, + "sparseFileWithMultipleAllocatedRanges": func(t *testing.T) string { + path := filepath.Join(t.TempDir(), "foo.txt") + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0644) + if err != nil { + t.Fatal(err) + } + defer f.Close() + setSparse(t, f) + if _, err = f.Write([]byte("testing 1 2 3\n")); err != nil { + t.Fatal(err) + } + // The documentation talks about FSCTL_SET_ZERO_DATA, but seeking also + // seems to create a hole. + if _, err = f.Seek(1000000, 0); err != nil { + t.Fatal(err) + } + if _, err = f.Write([]byte("more data later\n")); err != nil { + t.Fatal(err) + } + return path + }, + } { + t.Run(name, func(t *testing.T) { + path := setup(t) + f, err := os.Open(path) + if err != nil { + t.Fatal(err) + } + defer f.Close() + + fi, err := f.Stat() + if err != nil { + t.Fatal(err) + } + bi, err := winio.GetFileBasicInfo(f) + if err != nil { + t.Fatal(err) + } + + br := winio.NewBackupFileReader(f, true) + defer br.Close() + var buf bytes.Buffer + tw := tar.NewWriter(&buf) + err = WriteTarFileFromBackupStream(tw, br, f.Name(), fi.Size(), bi) + if err != nil { + t.Fatal(err) + } + tr := tar.NewReader(&buf) + hdr, err := tr.Next() + if err != nil { + t.Fatal(err) + } + + name, size, bi2, err := FileInfoFromHeader(hdr) + if err != nil { + t.Fatal(err) + } + if name != filepath.ToSlash(f.Name()) { + t.Errorf("got name %s, expected %s", name, filepath.ToSlash(f.Name())) + } + if size != fi.Size() { + t.Errorf("got size %d, expected %d", size, fi.Size()) + } + if !reflect.DeepEqual(*bi2, *bi) { + t.Errorf("got %#v, expected %#v", *bi2, *bi) + } + ensurePresent(t, hdr.PAXRecords, "MSWINDOWS.fileattr", "MSWINDOWS.rawsd") + // Reset file position so we can compare file contents. + // The file contents of the actual file should match what we get from the tar. + if _, err := f.Seek(0, 0); err != nil { + t.Fatal(err) + } + compareReaders(t, tr, f) + }) } +} - name, size, bi2, err := FileInfoFromHeader(hdr) +func TestZeroReader(t *testing.T) { + const size = 512 + var b [size]byte + var bExpected [size]byte + var r zeroReader + n, err := r.Read(b[:]) if err != nil { - t.Fatal(err) - } - - if name != filepath.ToSlash(f.Name()) { - t.Errorf("got name %s, expected %s", name, filepath.ToSlash(f.Name())) + t.Fatalf("Unexpected read error: %s", err) } - - if size != fi.Size() { - t.Errorf("got size %d, expected %d", size, fi.Size()) + if n != size { + t.Errorf("Wrong read size. got %d, expected %d", n, size) } - - if !reflect.DeepEqual(*bi2, *bi) { - t.Errorf("got %#v, expected %#v", *bi2, *bi) + for i := range b { + if b[i] != bExpected[i] { + t.Errorf("Wrong content at index %d. got %d, expected %d", i, b[i], bExpected[i]) + } } - - ensurePresent(t, hdr.PAXRecords, "MSWINDOWS.fileattr", "MSWINDOWS.rawsd") } From d255db16f698577ba43a7f8a7e127da38e43a4a9 Mon Sep 17 00:00:00 2001 From: Kyle Wojtaszek Date: Mon, 16 Aug 2021 14:03:06 -0700 Subject: [PATCH 4/4] FIxing corruption in callbacks Signed-off-by: Kyle Wojtaszek (cherry picked from commit 4c72048b5c311a321123e04846639552bc7c5a6a) Signed-off-by: Kevin Parsons --- pkg/etw/sample/sample.go | 3 +++ pkg/etw/wrapper_32.go | 8 ++++---- pkg/etw/wrapper_64.go | 4 ++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/pkg/etw/sample/sample.go b/pkg/etw/sample/sample.go index 0424fb86..fd315965 100644 --- a/pkg/etw/sample/sample.go +++ b/pkg/etw/sample/sample.go @@ -7,6 +7,7 @@ import ( "bufio" "fmt" "os" + "runtime" "github.com/Microsoft/go-winio/pkg/etw" "github.com/Microsoft/go-winio/pkg/guid" @@ -18,6 +19,8 @@ func callback(sourceID guid.GUID, state etw.ProviderState, level etw.Level, matc } func main() { + fmt.Printf("Running on %s/%s\n", runtime.GOOS, runtime.GOARCH) + group, err := guid.FromString("12341234-abcd-abcd-abcd-123412341234") if err != nil { logrus.Error(err) diff --git a/pkg/etw/wrapper_32.go b/pkg/etw/wrapper_32.go index 9b66cee6..6867a1f8 100644 --- a/pkg/etw/wrapper_32.go +++ b/pkg/etw/wrapper_32.go @@ -59,9 +59,9 @@ func eventSetInformation( // For x86, the matchAny and matchAll keywords need to be assembled from two // 32-bit integers, because the max size of an argument is uintptr, but those // two arguments are actually 64-bit integers. -func providerCallbackAdapter(sourceID *guid.GUID, state uint32, level uint8, matchAnyKeyword_low uint32, matchAnyKeyword_high uint32, matchAllKeyword_low uint32, matchAllKeyword_high uint32, filterData uintptr, i uintptr) uintptr { - matchAnyKeyword := uint64(matchAnyKeyword_high) << 32 | uint64(matchAnyKeyword_low) - matchAllKeyword := uint64(matchAllKeyword_high) << 32 | uint64(matchAllKeyword_low) +func providerCallbackAdapter(sourceID *guid.GUID, state uint32, level uint32, matchAnyKeyword_low uint32, matchAnyKeyword_high uint32, matchAllKeyword_low uint32, matchAllKeyword_high uint32, filterData uintptr, i uintptr) uintptr { + matchAnyKeyword := uint64(matchAnyKeyword_high)<<32 | uint64(matchAnyKeyword_low) + matchAllKeyword := uint64(matchAllKeyword_high)<<32 | uint64(matchAllKeyword_low) providerCallback(*sourceID, ProviderState(state), Level(level), uint64(matchAnyKeyword), uint64(matchAllKeyword), filterData, i) return 0 -} \ No newline at end of file +} diff --git a/pkg/etw/wrapper_64.go b/pkg/etw/wrapper_64.go index 1969ade0..fe83df2b 100644 --- a/pkg/etw/wrapper_64.go +++ b/pkg/etw/wrapper_64.go @@ -46,7 +46,7 @@ func eventSetInformation( // for provider notifications. Because Go has trouble with callback arguments of // different size, it has only pointer-sized arguments, which are then cast to // the appropriate types when calling providerCallback. -func providerCallbackAdapter(sourceID *guid.GUID, state uint32, level uint8, matchAnyKeyword uintptr, matchAllKeyword uintptr, filterData uintptr, i uintptr) uintptr { +func providerCallbackAdapter(sourceID *guid.GUID, state uintptr, level uintptr, matchAnyKeyword uintptr, matchAllKeyword uintptr, filterData uintptr, i uintptr) uintptr { providerCallback(*sourceID, ProviderState(state), Level(level), uint64(matchAnyKeyword), uint64(matchAllKeyword), filterData, i) return 0 -} \ No newline at end of file +}