windows: fix -d=checkptr slice failures #65

rikysya · 2020-03-25T15:41:23Z

This CL fixes unsafe casts to slices that are missing length or capacity.

Running tests with -d=checkptr enabled may panic on casting unsafe.Pointer to a static array of large predefined length, that is most likely much bigger than the size of the actual array in memory. Checkptr check is not satisfied if slicing operator misses length and capacity arguments (*[(1 << 30) - 1]uint16)(unsafe.Pointer(p))[:], or when there is no slicing at all (*[(1 << 30) - 1]uint16)(unsafe.Pointer(p)).

To find all potential cases I used grep -nr ")(unsafe.Pointer(" ./windows, then filtered out safe casts when object size is always static and known at compile time.

To reproduce the issue run tests with checkptr enabled go test -a -gcflags=all=-d=checkptr ./windows/....

Updates golang/go#34972
Fixes golang/go#38355

googlebot · 2020-03-25T15:41:30Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

rikysya · 2020-03-25T15:48:31Z

@googlebot I signed it!

googlebot · 2020-03-25T15:48:41Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

gopherbot · 2020-03-25T15:50:04Z

This PR (HEAD: 8f8c737) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sys/+/225418 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-03-25T15:51:23Z

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-03-25T16:10:50Z

This PR (HEAD: 212a4e6) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sys/+/225418 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-03-25T17:56:29Z

Message from Tobias Klauser:

Patch Set 3:

(2 comments)

Thanks for the CL. I'd suggest to split this up into two CLs: one addressing the golang.org/x/sys/unix package and one addressing the golang.org/x/sys/windows package.

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-03-26T12:48:45Z

This PR (HEAD: 9e3e00a) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sys/+/225418 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-03-26T13:40:43Z

Message from Tobias Klauser:

Patch Set 5:

(2 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-03-26T13:52:38Z

Message from Tobias Klauser:

Patch Set 7: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-03-26T13:52:38Z

Message from Gobot Gobot:

Patch Set 7:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=a8670abf

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-03-26T14:04:22Z

Message from Gobot Gobot:

Patch Set 7: TryBot-Result-1

1 of 14 TryBots failed:
Failed on android-amd64-emu: https://storage.googleapis.com/go-build-log/2975b27b/android-amd64-emu_77b6617e.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-03-26T14:08:23Z

Message from Tobias Klauser:

Patch Set 7:

Alex, could you please have a look at this if you find time? Thanks!

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-03-28T15:15:15Z

This PR (HEAD: 2ff63b3) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sys/+/225418 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-03-29T05:39:10Z

Message from Alex Brainman:

Patch Set 8:

(7 comments)

Thank you Yaroslav for working on this.

Please, see some comments below.

Alex

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-11T02:36:20Z

Message from Bryan C. Mills:

Patch Set 8:

(5 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-11T02:37:34Z

Message from Bryan C. Mills:

Patch Set 8: Run-TryBot+1

TRY=windows-amd64-race

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-11T02:37:35Z

Message from Gobot Gobot:

Patch Set 8:

SlowBots beginning. Status page: https://farmer.golang.org/try?commit=4d9c8109

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-11T02:41:18Z

Message from Gobot Gobot:

Patch Set 8: TryBot-Result+1

SlowBots are happy.
SlowBot builds that ran:

windows-amd64-race

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-11T03:16:04Z

Message from Alex Brainman:

Patch Set 8:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-11T03:32:21Z

Message from Bryan C. Mills:

Patch Set 8:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-12T02:29:08Z

Message from Alex Brainman:

Patch Set 8:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-14T22:22:27Z

Message from Bryan C. Mills:

Patch Set 8:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-29T17:51:18Z

This PR (HEAD: 2215e0a) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sys/+/225418 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-05-01T03:20:59Z

Message from Bryan C. Mills:

Patch Set 16: Code-Review+1

(5 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-05-03T08:15:43Z

Message from Alex Brainman:

Patch Set 16:

(5 comments)

Sorry I am slow to review. But this CL is large.

Alex

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-05-03T12:12:38Z

Message from Tobias Klauser:

Patch Set 16:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-05-04T10:24:45Z

Message from Alex Brainman:

Patch Set 16:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-05-04T16:39:23Z

Message from Bryan C. Mills:

Patch Set 16:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-05-05T11:05:03Z

This PR (HEAD: 43e62db) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sys/+/225418 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-05-05T11:49:50Z

This PR (HEAD: 77dff96) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sys/+/225418 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-05-05T11:53:28Z

Message from Yaroslav Vorobiov:

Patch Set 18:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-05-09T05:32:08Z

Message from Alex Brainman:

Patch Set 18:

(2 comments)

Just couple more comments. Thank you very much.

Alex

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

Ensure passing chkptr(-gcflags=all=-d=checkptr) check introduced in go 1.14

… NULL terminator

…String

gopherbot · 2020-05-18T12:52:06Z

This PR (HEAD: 73288ad) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/sys/+/225418 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-05-19T10:51:23Z

Message from Alex Brainman:

Patch Set 19: Run-TryBot+1

TRY=windows-386, windows-amd64, windows-amd64-race

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-05-19T10:52:49Z

Message from Gobot Gobot:

Patch Set 19:

SlowBots beginning. Status page: https://farmer.golang.org/try?commit=9a4b5ce9

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-05-19T10:55:40Z

Message from Gobot Gobot:

Patch Set 19: TryBot-Result+1

SlowBots are happy.
SlowBot builds that ran:

windows-386-2008
windows-amd64-2016
windows-amd64-race

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

This CL fixes unsafe casts to slices that are missing length or capacity. Running tests with -d=checkptr enabled may panic on casting unsafe.Pointer to a static array of large predefined length, that is most likely much bigger than the size of the actual array in memory. Checkptr check is not satisfied if slicing operator misses length and capacity arguments `(*[(1 << 30) - 1]uint16)(unsafe.Pointer(p))[:]`, or when there is no slicing at all `(*[(1 << 30) - 1]uint16)(unsafe.Pointer(p))`. To find all potential cases I used `grep -nr ")(unsafe.Pointer(" ./windows`, then filtered out safe casts when object size is always static and known at compile time. To reproduce the issue run tests with checkptr enabled `go test -a -gcflags=all=-d=checkptr ./windows/...`. Updates golang/go#34972 Fixes golang/go#38355 Change-Id: I9dd2084b4f9fb7618cdb140fb2f38b56b6d6cc04 GitHub-Last-Rev: 73288ad GitHub-Pull-Request: #65 Reviewed-on: https://go-review.googlesource.com/c/sys/+/225418 Run-TryBot: Alex Brainman <alex.brainman@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Alex Brainman <alex.brainman@gmail.com>

gopherbot · 2020-05-19T10:59:00Z

Message from Alex Brainman:

Patch Set 19: Code-Review+2

LGTM

Thank you very much.

Alex

Please don’t reply on this GitHub thread. Visit golang.org/cl/225418.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-05-19T11:00:25Z

This PR is being closed because golang.org/cl/225418 has been merged.

googlebot added the cla: no label Mar 25, 2020

googlebot added cla: yes and removed cla: no labels Mar 25, 2020

rikysya force-pushed the sys-ensure-chkptr branch from 8f8c737 to 212a4e6 Compare March 25, 2020 16:09

rikysya changed the title ~~sys: ensure passing checkptr check~~ sys/windows: ensure passing checkptr check Mar 26, 2020

rikysya changed the title ~~sys/windows: ensure passing checkptr check~~ sys/windows: fix -d=checkptr slice failures Mar 26, 2020

rikysya changed the title ~~sys/windows: fix -d=checkptr slice failures~~ windows: fix -d=checkptr slice failures Mar 26, 2020

rikysya force-pushed the sys-ensure-chkptr branch from 2215e0a to 43e62db Compare May 5, 2020 11:03

rikysya added 10 commits May 18, 2020 15:50

sys: ensure passing chkptr check

d7147e9

Ensure passing chkptr(-gcflags=all=-d=checkptr) check introduced in go 1.14

use UTF16PtrToString function instead of slicing

05e35ff

revert unix changes

d3ae10d

windows/svc: use UTF16PtrToString in manager config

6335a67

windows/svc: fix manager config toStringSlice pointer casting

033a5e1

windows: UTF16PtrToString remove max parameter

776a65d

windows: UTF16PtrToString use strings builder

7cbfa12

windows: use unsafeheader.Slice

0e31a5e

windows: UTF16PtrToString explicitly return empty string for a ptr to…

73c410a

… NULL terminator

windows: refactor (*SD).copySelfRelativeSecurityDescriptor and (*SD).…

73288ad

…String

rikysya force-pushed the sys-ensure-chkptr branch from 77dff96 to 73288ad Compare May 18, 2020 12:50

gopherbot closed this May 19, 2020

windows: fix -d=checkptr slice failures #65

windows: fix -d=checkptr slice failures #65

Conversation

rikysya commented Mar 25, 2020 • edited Loading

googlebot commented Mar 25, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

rikysya commented Mar 25, 2020

googlebot commented Mar 25, 2020

gopherbot commented Mar 25, 2020

gopherbot commented Mar 25, 2020

gopherbot commented Mar 25, 2020

gopherbot commented Mar 25, 2020

gopherbot commented Mar 26, 2020

gopherbot commented Mar 26, 2020

gopherbot commented Mar 26, 2020

gopherbot commented Mar 26, 2020

gopherbot commented Mar 26, 2020

gopherbot commented Mar 26, 2020

gopherbot commented Mar 28, 2020

gopherbot commented Mar 29, 2020

gopherbot commented Apr 11, 2020

gopherbot commented Apr 11, 2020

gopherbot commented Apr 11, 2020

gopherbot commented Apr 11, 2020

gopherbot commented Apr 11, 2020

gopherbot commented Apr 11, 2020

gopherbot commented Apr 12, 2020

gopherbot commented Apr 14, 2020

gopherbot commented Apr 29, 2020

gopherbot commented May 1, 2020

gopherbot commented May 3, 2020

gopherbot commented May 3, 2020

gopherbot commented May 4, 2020

gopherbot commented May 4, 2020

gopherbot commented May 5, 2020

gopherbot commented May 5, 2020

gopherbot commented May 5, 2020

gopherbot commented May 9, 2020

gopherbot commented May 18, 2020

gopherbot commented May 19, 2020

gopherbot commented May 19, 2020

gopherbot commented May 19, 2020

gopherbot commented May 19, 2020

gopherbot commented May 19, 2020

rikysya commented Mar 25, 2020 •

edited

Loading