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

syscall: verify that windows uintptr structure fields are not pointers #24820

Closed
alexbrainman opened this issue Apr 12, 2018 · 4 comments
Closed

syscall: verify that windows uintptr structure fields are not pointers #24820

alexbrainman opened this issue Apr 12, 2018 · 4 comments

Comments

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Apr 12, 2018

Recently #21376 (comment) we discovered that using uintptr structure fields to store pointers will confuse GC.

https://go-review.googlesource.com/#/c/go/+/106275/ converts all syscall code used by crypto/x509 package to use syscall.Pointer instead of uintptr. But there are other uintptr fields in syscall.

This issue is so we don't forget to check them all.

Alex

@andybons andybons added the NeedsFix label Apr 12, 2018
@andybons andybons added this to the Unplanned milestone Apr 12, 2018
@aclements aclements modified the milestones: Unplanned, Go1.11 Apr 12, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 17, 2018

Change https://golang.org/cl/106275 mentions this issue: syscall: introduce Pointer type and use it instead of uintptr

gopherbot pushed a commit that referenced this issue Apr 18, 2018
Some syscall structures used by crypto/x509 have uintptr
fields that store pointers. These pointers are set with
a pointer to another Go structure. But the pointers are
not visible by garbage collector, and GC does not update
the fields after they were set. So when structure with
invalid uintptr pointers passed to Windows, we get
memory corruption.

This CL introduces CertInfo, CertTrustListInfo and
CertRevocationCrlInfo types. It uses pointers to new types
instead of uintptr in CertContext, CertSimpleChain and
CertRevocationInfo.

CertRevocationInfo, CertChainPolicyPara and
CertChainPolicyStatus types have uintptr field that can
be pointer to many different things (according to Windows
API). So this CL introduces Pointer type to be used for
those cases.

As suggested by Austin Clements.

Fixes #21376
Updates #24820

Change-Id: If95cd9eee3c69e4cfc35b7b25b1b40c2dc8f0df7
Reviewed-on: https://go-review.googlesource.com/106275
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@alexbrainman
Copy link
Member Author

@alexbrainman alexbrainman commented May 5, 2018

@aclements I spent some time looking at windows uintptrs in syscall package.

And the only real pointer remains is AddrinfoW.Addr (see https://msdn.microsoft.com/en-us/library/windows/desktop/ms737529(v=vs.85).aspx for its documentation). I tried changing AddrinfoW.Addr type from uintptr into syscall.Pointer, and nothing else needs to change anywhere in standard library. We do use AddrinfoW.Addr in net package in 2 places, but we already convert all uses of it into unsafe.Pointer. Also the way we use AddrinfoW.Addr it is always set by Windows, never by us, so, while it is, probably, pointing onto Go memory, it could be treated as external memory by garbage collector. I think.

Given we already changed syscall package in CL 106275, I think we should change AddrinfoW.Addr into syscall.Pointer. But leaving this decision up to you. Let me know what you think either way. Happy to send a CL if you agree.

Thank you.

Alex

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 29, 2018

ping @aclements

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 11, 2018

Change https://golang.org/cl/123455 mentions this issue: syscall: convert Windows AddrinfoW.Addr from uintptr to syscall.Pointer

@gopherbot gopherbot closed this in becd2a8 Jul 12, 2018
@golang golang locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.