Update to use upstream sddl/SecurityAttribute but retain old exported functions #172

katiewasnothere · 2020-06-02T02:12:05Z

This PR moves go-winio passed breaking changes from /x/sys/windows for dealing with Security Descriptors.

Added test to validate flow change to use windows.SECURITY_DESCRIPTOR.

Signed-off-by: Kathryn Baldauf kabaldau@microsoft.com

kevpar · 2020-06-02T04:13:19Z

pipe.go

-			defer localFree(sdb)
-			copy((*[0xffff]byte)(unsafe.Pointer(sdb))[:], sd)
+			defer windows.LocalFree(windows.Handle(sdb))
+			copy((*[0xffff]byte)(unsafe.Pointer(sdb))[:len], (*[0xffff]byte)(unsafe.Pointer(sd))[:len])


This looks like we are copying the security descriptor into a new buffer and then casting that buffer to be a security descriptor as well. Should we just change objectAttributes to take a windows.SECURITY_DESCRIPTOR instead?

We're fairly inconsistent about following the Go requirements around the use of Go pointers in FFI calls--would we face any possible object lifetime issues by doing this?

jstarks · 2020-06-02T04:17:00Z

What does this change actually solve?

katiewasnothere · 2020-06-02T17:25:40Z

What does this change actually solve?

This really just moves the repo passed the breaking changes made to x/sys/windows. Not super critical.

kolyshkin · 2020-06-03T01:48:11Z

backuptar/tar.go

 			return nil, err
 		}
-		_, err = bw.Write(sd)
+		_, err = bw.Write((*[0xffff]byte)(unsafe.Pointer(sd))[:sdLen])


I think this needs a comment to explain where 0xffff comes from

kolyshkin · 2020-06-03T01:48:31Z

go.sum

 golang.org/x/sys v0.0.0-20190916202348-b4ddaad3f8a3 h1:7TYNF4UdlohbFwpNH04CoPMp1cHUZgO1Ebq5r2hIjfo=
 golang.org/x/sys v0.0.0-20190916202348-b4ddaad3f8a3/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20200523222454-059865788121 h1:rITEj+UZHYC927n8GT97eC3zrpzXdb/voyeOuVKS46o=
+golang.org/x/sys v0.0.0-20200523222454-059865788121/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=


did you run go mod tidy?

thaJeztah · 2020-09-28T13:37:17Z

@katiewasnothere looks like this needs a rebase now that #175 was merged

TBBle · 2021-02-11T17:12:21Z

@katiewasnothere looks like this needs a rebase now that #175 was merged

A quick look suggests the conflicts with #175 are just adjacent-line changes in WriteBackupStreamFromTarFile and the imports list.

… functions Signed-off-by: Kathryn Baldauf <kabaldau@microsoft.com>

katiewasnothere · 2021-03-13T00:28:48Z

Updated. @dcantah and/or @kevpar could you take another look here?

dcantah · 2021-03-13T00:31:06Z

vhd/vhd.go

+//sys createVirtualDisk(virtualStorageType *VirtualStorageType, path string, virtualDiskAccessMask uint32, securityDescriptor *windows.SECURITY_DESCRIPTOR, 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 attachVirtualDisk(handle syscall.Handle, securityDescriptor *windows.SECURITY_DESCRIPTOR, attachVirtualDiskFlag uint32, providerSpecificFlags uint32, parameters *AttachVirtualDiskParameters, overlapped *syscall.Overlapped) (err error) [failretval != 0] = virtdisk.AttachVirtualDisk


@TBBle The signature for these is changing in this PR anyways so let's make sure we get in the other /x/sys/windows'ify PR shortly after and we can cut a release.

Sounds good. I did a quick review previously, and the conflicts are textual-only. i.e. on this line I changed the type of handle and overlapped only.

So assuming this lands first, rebasing #197 should only take a few minutes, mostly compile-checking everything.

dcantah · 2021-03-13T00:45:27Z

pipe.go

-
-			sdb := &securityDescriptor{
-				Revision: 1,
-				Control:  cSE_DACL_PRESENT,


We don't want to set windows.SE_DACL_PRESENT anymore?

TBBle · 2021-03-13T01:53:38Z

backuptar/tar.go

 			return nil, err
 		}
-		_, err = bw.Write(sd)
+		_, err = bw.Write((*[(1 << 31) - 1]byte)(unsafe.Pointer(sd))[:sdLen])


This idiom fails -gcflags=all=-d=checkptr as of Go 1.14. In this case, it's just being moved from elsewhere, so it's not an objection to this change, but a reminder that we need to do a pass over the code-base with checkptr (or race enabled, which includes it) with Go 1.15 or later, and fix occurrences of this.

The fix itself is pretty simple, I did the same pass for hcsshim in microsoft/hcsshim#926, see Uint16BufferToSlice.

This idiom fails -gcflags=all=-d=checkptr as of Go 1.14. In this case

Oh! It just occurred to me now that there's no CI running in this repository (other than the license/cla check); should we add a basic github-action?

thaJeztah · 2022-02-18T08:39:50Z

@katiewasnothere looks like this needs a rebase because #220 was merged

katiewasnothere mentioned this pull request Jun 2, 2020

go.mod: bump containerd/cgroups microsoft/hcsshim#828

Merged

kevpar reviewed Jun 2, 2020

View reviewed changes

katiewasnothere mentioned this pull request Jun 2, 2020

Move to upstream Sddl/SecurityAttribute functions #165

Open

kolyshkin reviewed Jun 3, 2020

View reviewed changes

thaJeztah mentioned this pull request Feb 11, 2021

Use x/sys/windows instead of syscall #197

Open

Update to use upstream sddl/SecurityAttribute but retain old exported…

f1eb81f

… functions Signed-off-by: Kathryn Baldauf <kabaldau@microsoft.com>

katiewasnothere force-pushed the update_sddl_securityattribute branch from 0a74186 to f1eb81f Compare March 13, 2021 00:25

katiewasnothere requested a review from a team as a code owner March 13, 2021 00:25

dcantah reviewed Mar 13, 2021

View reviewed changes

TBBle reviewed Mar 13, 2021

View reviewed changes

thaJeztah mentioned this pull request Apr 12, 2021

[v1.5.0 GA] hygiene: tag github.com/containerd/x modules before GA containerd/containerd#5345

Closed

14 tasks

Update to use upstream sddl/SecurityAttribute but retain old exported functions #172

Are you sure you want to change the base?

Update to use upstream sddl/SecurityAttribute but retain old exported functions #172

Uh oh!

Conversation

katiewasnothere commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jstarks commented Jun 2, 2020

Uh oh!

katiewasnothere commented Jun 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 28, 2020

Uh oh!

TBBle commented Feb 11, 2021

Uh oh!

katiewasnothere commented Mar 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TBBle Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

katiewasnothere commented Jun 2, 2020 •

edited

Loading

TBBle Mar 13, 2021 •

edited

Loading