[go] runtime: Use RtlGenRandom instead of CryptGenRandom

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

runtime: Use RtlGenRandom instead of CryptGenRandom

Patch Set 1: Run-TryBot+1

R=1.8

--
https://go-review.googlesource.com/22933
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

runtime: Use RtlGenRandom instead of CryptGenRandom

Patch Set 1:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=280cb325

--
https://go-review.googlesource.com/22933
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>

Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

runtime: Use RtlGenRandom instead of CryptGenRandom

Patch Set 1:

Build is still in progress...
This change failed on windows-386-gce:
See
https://storage.googleapis.com/go-build-log/280cb325/windows-386-gce_045bb639.log

Consult https://build.golang.org/ to see whether it's a new failure. Other
builds still in progress; subsequent failure notices suppressed until final
report.

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

runtime: Use RtlGenRandom instead of CryptGenRandom

Patch Set 1: TryBot-Result-1

1 of 18 TryBots failed:
Failed on windows-386-gce:
https://storage.googleapis.com/go-build-log/280cb325/windows-386-gce_045bb639.log

Consult https://build.golang.org/ to see whether they are new failures.

[Minux Ma (Gerrit)]

Minux Ma has posted comments on this change.

runtime: Use RtlGenRandom instead of CryptGenRandom

Patch Set 1:

(1 comment)

https://go-review.googlesource.com/#/c/22933/1/src/runtime/os_windows.go
File src/runtime/os_windows.go:

PS1, Line 23: SystemFunction036
According to its MSDN docs
https://msdn.microsoft.com/en-us/library/windows/desktop/aa387694(v=vs.85).aspx:

[The RtlGenRandom function is available for use in the operating systems
specified in the Requirements section. It may be altered or unavailable in
subsequent versions. Instead, use the CryptGenRandom function.]


Should we rely on such APIs?

--
https://go-review.googlesource.com/22933
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: Minux Ma <mi...@golang.org>
Gerrit-HasComments: Yes

[John Starks (Gerrit)]

John Starks uploaded a change:
https://go-review.googlesource.com/22933

runtime: Use RtlGenRandom instead of CryptGenRandom

This change replaces the use of CryptGenRandom with RtlGenRandom in
Windows to generate cryptographically random numbers during process
startup. RtlGenRandom uses the same RNG as CryptGenRandom, but it has many
fewer DLL dependencies and so does not affect process startup time as
much.

This reduces simple process startup time from 22ms to 20ms on my machine.

Fixes #15589

Change-Id: Ie97ccfe6d7dbe9abe2a0bf6906177fe8ae4195fb
---
M src/runtime/os_windows.go
1 file changed, 4 insertions(+), 16 deletions(-)



diff --git a/src/runtime/os_windows.go b/src/runtime/os_windows.go
index 9147091..1ffa511 100644
--- a/src/runtime/os_windows.go
+++ b/src/runtime/os_windows.go
@@ -20,9 +20,7 @@
//go:cgo_import_dynamic runtime._CreateIoCompletionPort
CreateIoCompletionPort%4 "kernel32.dll"
//go:cgo_import_dynamic runtime._CreateThread CreateThread%6 "kernel32.dll"
//go:cgo_import_dynamic runtime._CreateWaitableTimerA
CreateWaitableTimerA%3 "kernel32.dll"
-//go:cgo_import_dynamic runtime._CryptAcquireContextW
CryptAcquireContextW%5 "advapi32.dll"
-//go:cgo_import_dynamic runtime._CryptGenRandom
CryptGenRandom%3 "advapi32.dll"
-//go:cgo_import_dynamic runtime._CryptReleaseContext
CryptReleaseContext%2 "advapi32.dll"
+//go:cgo_import_dynamic runtime._RtlGenRandom
SystemFunction036%2 "advapi32.dll"
//go:cgo_import_dynamic runtime._DuplicateHandle
DuplicateHandle%7 "kernel32.dll"
//go:cgo_import_dynamic runtime._ExitProcess ExitProcess%1 "kernel32.dll"
//go:cgo_import_dynamic runtime._FreeEnvironmentStringsW
FreeEnvironmentStringsW%1 "kernel32.dll"
@@ -67,9 +65,7 @@
_CreateIoCompletionPort,
_CreateThread,
_CreateWaitableTimerA,
- _CryptAcquireContextW,
- _CryptGenRandom,
- _CryptReleaseContext,
+ _RtlGenRandom,
_DuplicateHandle,
_ExitProcess,
_FreeEnvironmentStringsW,
@@ -265,17 +261,9 @@

//go:nosplit
func getRandomData(r []byte) {
- const (
- prov_rsa_full = 1
- crypt_verifycontext = 0xF0000000
- )
- var handle uintptr
n := 0
- if stdcall5(_CryptAcquireContextW, uintptr(unsafe.Pointer(&handle)), 0,
0, prov_rsa_full, crypt_verifycontext) != 0 {
- if stdcall3(_CryptGenRandom, handle, uintptr(len(r)),
uintptr(unsafe.Pointer(&r[0]))) != 0 {
- n = len(r)
- }
- stdcall2(_CryptReleaseContext, handle, 0)
+ if stdcall2(_RtlGenRandom, uintptr(unsafe.Pointer(&r[0])),
uintptr(len(r)))&0xff != 0 {
+ n = len(r)
}
extendRandom(r, n)
}

--
https://go-review.googlesource.com/22933

[John Starks (Gerrit)]

John Starks has posted comments on this change.

runtime: Use RtlGenRandom instead of CryptGenRandom

Patch Set 1:

> (1 comment)

Despite the documentation, RtlGenRandom is used heavily inside Windows and
in popular open source projects (such as Chromium). It cannot realistically
be altered or removed at this point.

--
https://go-review.googlesource.com/22933
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

Gerrit-Reviewer: John Starks <jost...@microsoft.com>
Gerrit-Reviewer: Minux Ma <mi...@golang.org>
Gerrit-HasComments: No

[Alex Brainman (Gerrit)]

Alex Brainman has posted comments on this change.

runtime: Use RtlGenRandom instead of CryptGenRandom

Patch Set 1:

> 1 of 18 TryBots failed:
> Failed on windows-386-gce:
https://storage.googleapis.com/go-build-log/280cb325/windows-386-gce_045bb639.log
>
> Consult https://build.golang.org/ to see whether they are new
> failures.

Life is never simple. Is it?

Looking at the error message:

```
--- FAIL: TestCgoCrashHandler (5.40s)
crash_test.go:66: building testprogcgo: exit status 2
# _/C_/workdir/go/src/runtime/testdata/testprogcgo
.\threadpanic_windows.c: In function 'start':
.\threadpanic_windows.c:21:26: warning: passing argument 3
of '_beginthreadex' from incompatible pointer type
if(_beginthreadex(0, 0, die, 0, 0, 0) != 0)
^
In file included from .\threadpanic_windows.c:5:0:
C:/TDM-GCC-32/include/process.h:100:2: note: expected 'unsigned int
(__attribute__((__stdcall__)) *)(void *)' but argument is of type 'unsigned
int (*)(void *)'
_beginthreadex (void *, unsigned, unsigned (__stdcall *) (void *),
^
```

this is just a warning that we normally don't get to see. Filed #15623 for
that.

But

```
# _/C_/workdir/go/src/runtime/testdata/testprogcgo
C:\workdir\go\pkg\tool\windows_386\link.exe: running gcc failed: exit
status 1
C:\Users\WINGOP~1\AppData\Local\Temp\go-link-196108955\go.o:(.data+0xb7b4):
undefined reference to `SystemFunction036@8'
collect2.exe: error: ld returned 1 exit status
```

is real. When we do CGO, we use gcc to link final executable. It appears
the gcc cannot find SystemFunction036 in advapi32.dll. I think it is
because SystemFunction036 is not listed in libadvapi32.a

```
C:\dev\tdm_gcc_32_5.1.0\lib>objdump -t libadvapi32.a | grep SystemFunction03
File STDIN:
[ 7](sec 1)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000
_SystemFunction034@12
[ 8](sec 5)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000
__imp__SystemFunction034@12
[ 7](sec 1)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000
_SystemFunction033@8
[ 8](sec 5)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000
__imp__SystemFunction033@8
[ 7](sec 1)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000
_SystemFunction032@8
[ 8](sec 5)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000
__imp__SystemFunction032@8
[ 7](sec 1)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000
_SystemFunction031@8
[ 8](sec 5)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000
__imp__SystemFunction031@8
[ 7](sec 1)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000
_SystemFunction030@8
[ 8](sec 5)(fl 0x00)(ty 0)(scl 2) (nx 0) 0x00000000
__imp__SystemFunction030@8

C:\dev\tdm_gcc_32_5.1.0\lib>
```

For whatever reason. And I use fairly recent version of mingw.

I don't see how we can make SystemFunction036 work with CGO. Perhaps we
should keep our current code for CGO, and use SystemFunction036 for pure Go
programs.

Maybe something else.

What do you all think?

Alex

--
https://go-review.googlesource.com/22933
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>

[Alex Brainman (Gerrit)]

Alex Brainman has posted comments on this change.

runtime: Use RtlGenRandom instead of CryptGenRandom

Patch Set 1:

> I don't see how we can make SystemFunction036 work with CGO.
> Perhaps we should keep our current code for CGO, and use
> SystemFunction036 for pure Go programs.
>

In fact windows-amd64 CGO is fine. So we just need a special case for
windows-386 CGO only.

[John Starks (Gerrit)]

John Starks has posted comments on this change.

runtime: Use RtlGenRandom instead of CryptGenRandom

Patch Set 1:

> >
> > I don't see how we can make SystemFunction036 work with CGO.
> > Perhaps we should keep our current code for CGO, and use
> > SystemFunction036 for pure Go programs.
> >
>
> In fact windows-amd64 CGO is fine. So we just need a special case
> for windows-386 CGO only.
>
> Alex

I think I understand why this is happening on 386. SystemFunction036 is
mangled as cdecl even though it is stdcall. I will push what I think will
fix it.

[John Starks (Gerrit)]

Reviewers: Gobot Gobot, Brad Fitzpatrick

John Starks uploaded a new patch set:
https://go-review.googlesource.com/22933

runtime: use RtlGenRandom instead of CryptGenRandom

This change replaces the use of CryptGenRandom with RtlGenRandom in
Windows to generate cryptographically random numbers during process
startup. RtlGenRandom uses the same RNG as CryptGenRandom, but it has many
fewer DLL dependencies and so does not affect process startup time as
much.

This reduces simple process startup time from 22ms to 20ms on my machine.

Fixes #15589

Change-Id: Ie97ccfe6d7dbe9abe2a0bf6906177fe8ae4195fb
---
M src/runtime/os_windows.go
1 file changed, 4 insertions(+), 16 deletions(-)

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

runtime: use RtlGenRandom instead of CryptGenRandom

Patch Set 2: Run-TryBot+1

--
https://go-review.googlesource.com/22933
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: John Starks <jost...@microsoft.com>
Gerrit-Reviewer: Minux Ma <mi...@golang.org>

Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

runtime: use RtlGenRandom instead of CryptGenRandom

Patch Set 2:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=4824497e

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

runtime: use RtlGenRandom instead of CryptGenRandom

Patch Set 2:

Build is still in progress...
This change failed on windows-386-gce:
See

https://storage.googleapis.com/go-build-log/4824497e/windows-386-gce_4905f1f8.log

Consult https://build.golang.org/ to see whether it's a new failure. Other
builds still in progress; subsequent failure notices suppressed until final
report.

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

runtime: use RtlGenRandom instead of CryptGenRandom

Patch Set 2: TryBot-Result-1

1 of 18 TryBots failed:
Failed on windows-386-gce:

https://storage.googleapis.com/go-build-log/4824497e/windows-386-gce_4905f1f8.log

Consult https://build.golang.org/ to see whether they are new failures.

[Alex Brainman (Gerrit)]

Alex Brainman has posted comments on this change.

runtime: use RtlGenRandom instead of CryptGenRandom

Patch Set 2:

(1 comment)

I still see no way to advance this, but move old code into
os_cgo_windows_386.go (or whatever to make it run only on windows-386 and
cgo). We will, obviously, have to supply whatever is needed on other
windows platforms (!windows-386-cgo) so it still builds.

Alex

https://go-review.googlesource.com/#/c/22933/2/src/runtime/os_windows.go
File src/runtime/os_windows.go:

Line 23: //go:cgo_import_dynamic runtime._RtlGenRandom
SystemFunction036 "advapi32.dll"
s/SystemFunction036/SystemFunction036%2/

your original code theoretically looks correct.

--
https://go-review.googlesource.com/22933
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: John Starks <jost...@microsoft.com>
Gerrit-Reviewer: Minux Ma <mi...@golang.org>

Gerrit-HasComments: Yes

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

runtime: use RtlGenRandom instead of CryptGenRandom

Patch Set 2:

R=go1.8

(the correct syntax)

--
https://go-review.googlesource.com/22933
Gerrit-Reviewer: Alex Brainman <alex.b...@gmail.com>
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: John Starks <jost...@microsoft.com>
Gerrit-Reviewer: Minux Ma <mi...@golang.org>

Gerrit-HasComments: No