Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/sys/windows: ProcThreadAttributeListContainer not working with Windows pseudo console #50134

Closed
dcantah opened this issue Dec 13, 2021 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@dcantah
Copy link

dcantah commented Dec 13, 2021

What version of Go are you using (go version)?

$ go version 
1.17.2

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
C:\Users\dcanter>go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\dcanter\AppData\Local\go-build
set GOENV=C:\Users\dcanter\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\dcanter\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\dcanter\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.17.2
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=NUL
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\dcanter\AppData\Local\Temp\go-build1935140407=/tmp/go-build -gno-record-gcc-switches

What did you do?

Pass a pseudo console handle to the Update function of windows.ProcThreadAttributeListContainer. The call succeeds, however on creation of the process shortly after launch the process exits with exit code 3221225794 (0xc0000142) which is documented in the Note section slightly below "Creating the Hosted Process" https://docs.microsoft.com/en-us/windows/console/creating-a-pseudoconsole-session#creating-the-hosted-process.

The two documented ways the process would return this exit code is if either the handle is invalid or the pseudo console session is closed during process startup (calling ClosePseudoConsole). The latter doesn't seem to be the case, so my eyes turn towards somehow the handle getting mangled in the process somewhere. The HPC type is just an opaque pointer to an internal structure I believe.

Something to note, I'd tried a couple different bindings for PROC_THREAD_ATTRIBUTE_LIST and it's family of functions, most of which turn out alright and starting the process succeeds. Notably from testing, a recreation of the stdlibs definition and methods for the attribute list work fine (the ones used in syscall.StartProcess https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/syscall/exec_windows.go;l=351). What's odd to me is I've tried out 4 or 5 of the attributes that I'd like to make use of, and all of them work fine with the x/sys/windows and stdlib approach. The pseudo console attribute is the only one that seems to cause any trouble, and only for the x/sys/windows definition.

Here's a working program with the stdlib definitions: https://github.com/dcantah/hcsshim/blob/try-pseudo-console/internal/exec/tryexec/main.go

Here's a program that fails with the aforementioned exit code, where the only difference is the swap of attr list definitions: https://github.com/dcantah/hcsshim/blob/try-pseudo-console-xsys/internal/exec/tryexec/main.go

The above programs will need to be run on Windows version 1809 (windows server 2019) or higher.

What did you expect to see?

Process starts and doesn't exit immediately with exit code 3221225794.

What did you see instead?

Process exits with exit code 3221225794.

cc @zx2c4. I'm gonna talk to the console folks to find some nice bps to debug this. Right now just checking in kernelbase!UpdateProcThreadAttribute and kernelbase!BasepConvertWin32AttributeList things seem fine but shrugs. Thought I'd tag you as you wrote the attr list code and have went through the mental gymnastics for the gc involvements here already.

@gopherbot gopherbot added this to the Unreleased milestone Dec 13, 2021
@zx2c4
Copy link
Contributor

zx2c4 commented Dec 13, 2021

image

Is that right? This will copy 8 bytes of the address at hPc into the list. Maybe what you meant instead was to pass &hPc?

// Update modifies the ProcThreadAttributeList using UpdateProcThreadAttribute.
// Note that the value passed to this function will be copied into memory
// allocated by LocalAlloc, the contents of which should not contain any
// Go-managed pointers, even if the passed value itself is a Go-managed
// pointer.
func (al *ProcThreadAttributeListContainer) Update(attribute uintptr, value unsafe.Pointer, size uintptr) error {
        alloc, err := LocalAlloc(LMEM_FIXED, uint32(size))
        if err != nil {
                return err
        }
        var src, dst []byte
        hdr := (*unsafeheader.Slice)(unsafe.Pointer(&src))
        hdr.Data = value
        hdr.Cap = int(size)
        hdr.Len = int(size)
        hdr = (*unsafeheader.Slice)(unsafe.Pointer(&dst))
        hdr.Data = unsafe.Pointer(alloc)
        hdr.Cap = int(size)
        hdr.Len = int(size)
        copy(dst, src)
        al.heapAllocations = append(al.heapAllocations, alloc)
        return updateProcThreadAttribute(al.data, 0, attribute, unsafe.Pointer(alloc), size, nil, nil)
}

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 13, 2021
@gopherbot
Copy link

Change https://golang.org/cl/371276 mentions this issue: windows: allocate attribute list with LocalAlloc, not individual items

@zx2c4
Copy link
Contributor

zx2c4 commented Dec 13, 2021

@dcantah Can you let me know if https://golang.org/cl/371276 fixes the problem for you?

zx2c4 referenced this issue in dcantah/hcsshim Dec 13, 2021
Changed attr list to x/sys/windows to showcase possible bug.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
@dcantah
Copy link
Author

dcantah commented Dec 13, 2021

@zx2c4 That did the trick yep! Appreciate the quick turnaround

@zx2c4
Copy link
Contributor

zx2c4 commented Dec 13, 2021

Oh goody gumdrops. I'll note that you tested it on the CL, and hopefully @bufflig can +2 it shortly so you can use it.

@golang golang locked and limited conversation to collaborators Dec 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants