All the finalizer-enabled C wrappers must be careful to use
runtime.KeepAlive to ensure the C wrapper object (a Go object)
lives through the end of every C call using state that the
wrapper's finalizer would free.
This CL makes the wrappers appropriately careful.
The test proves that this is the bug I was chasing in a
separate real program, and that the KeepAlives fix it.
I did not write a test of every possible operation.
Change-Id: I627007e480f16adf8396e7f796b54e5525d9ea80
Reviewed-on: https://go-review.googlesource.com/64870
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/65489
There's no way for a *.syso file to be compiled to work both in
normal mode and in msan mode. Assume they are compiled in
normal mode and drop them in msan mode.
Packages with syso files currently fail in -msan mode because
the syso file calls out to a routine like memcmp which then
falsely reports uninitialized memory. After this CL, they will fail
in -msan with link errors, because the syso will not be used.
But then it will at least be possible for package authors to write
fallback code in the package that avoids the syso in -msan mode,
so that the package with the syso can at least run in both modes.
Without this CL, that's not possible.
See #21884.
Change-Id: I77340614c4711325032484e65fa9c3f8332741d5
Reviewed-on: https://go-review.googlesource.com/63917
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/65488
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
The syso is not compiled with -fsanitize=memory, so don't try to use it.
Otherwise the first time it calls out to memcmp, memcmp complains
that it is being asked to compare uninitialized memory.
Change-Id: I85ab707cfbe64eded8e110d4d6b40d1b75f50541
Reviewed-on: https://go-review.googlesource.com/63916
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/65487
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
I've now debugged multiple mysterious "inability to communicate"
bugs that manifest as a silent unexplained authentication failure but are
really crypto.AEAD.Open being invoked with badly aligned buffers.
In #21624 I suggested using a panic as the consequence of bad alignment,
so that this kind of failure is loud and clearly different from, say, a
corrupted or invalid message signature. Adding the panic here made
my failure very easy to track down, once I realized that was the problem.
I don't want to debug another one of these.
Also using this CL as an experiment to get data about the impact of
maybe applying this change more broadly in the master branch.
Change-Id: Id2e2d8e980439f8acacac985fc2674f7c96c5032
Reviewed-on: https://go-review.googlesource.com/63915
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/65486
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
This matches the standard GenerateKey and more importantly Precompute,
so that if you generate a key and then store it, read it back, call Precompute
on the new copy, and then do reflect.DeepEqual on the two copies, they
will match. Before this CL, the original key had CRTValues == nil and the
reconstituted key has CRTValues != nil (but len(CRTValues) == 0).
Change-Id: I1ddc64342a50a1b65a48d827e4d564f1faab1945
Reviewed-on: https://go-review.googlesource.com/63914
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/65485
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
When using the go command, test binaries end in .test,
but when using Bazel, test binaries conventionally end in _test.
Change-Id: Ic4cac8722fd93ae316169f87b321f68e0b71f0c3
Reviewed-on: https://go-review.googlesource.com/63913
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/65484
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
In routines like GenerateKey, where bits from the randomness source have a
visible effect on the output, we bypass BoringCrypto if given a non-standard
randomness source (and also assert that this happens only during tests).
In the decryption paths, the randomness source is only for blinding and has
no effect on the output, so we unconditionally invoke BoringCrypto, letting it
use its own randomness source as it sees fit. This in turn lets us verify that
the non-BoringCrypto decryption function is never called, not even in tests.
Unfortunately, while the randomness source has no visible effect on the
decrypt operation, the decrypt operation does have a visible effect on
the randomness source. If decryption doesn't use the randomness source,
and it's a synthetic stream, then a future operation will read a different
position in the stream and may produce different output. This happens
in tests more often than you'd hope.
To keep behavior of those future operations unchanged while still
ensuring that the original decrypt is never called, this CL adds a
simulation of the blinding preparation, to discard the right amount
from the random source before invoking BoringCrypto.
Change-Id: If2f87b856c811b59b536187c93efa99a97721419
Reviewed-on: https://go-review.googlesource.com/63912
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/65483
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
This is documented to work (in hash.Hash's definition)
and existing code assumes it works. Add a test.
Change-Id: I63546f3b2d66222683a4f268a4eaff835fd836fe
Reviewed-on: https://go-review.googlesource.com/63911
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/65482
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
hmac.New returns a hash.Hash, which defines Sum as:
// Sum appends the current hash to b and returns the resulting slice.
// It does not change the underlying hash state.
Sum(b []byte) []byte
I've now seen two different pieces of code that make
use of the assumption that Sum has no effect on the
internal state, so make it so.
Test in next CL in stack, so that it can be cherry-picked
to master.
Change-Id: Iad84ab3e2cc12dbecef25c3fc8f2662d157b0d0b
Reviewed-on: https://go-review.googlesource.com/63910
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/65481
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
The standard Go crypto/rsa allows signatures to be shorter
than the RSA modulus and assumes leading zeros.
BoringCrypto does not, so supply the leading zeros explicitly.
This fixes the golang.org/x/crypto/openpgp tests.
Change-Id: Ic8b18d6beb0e02992a0474f5fdb2b73ccf7098cf
Reviewed-on: https://go-review.googlesource.com/62170
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/65480
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
Did not consider these fields being embedded or adopted
into structs defined in other packages, but that's possible too.
Refine the import path check to account for that.
Fixes 'go test -short golang.org/x/crypto/ssh' but also
adds a new test in internal/boring for the same problem.
Change-Id: Ied2d04fe2b0ac3b0a34f07bc8dfc50fc203abb9f
Reviewed-on: https://go-review.googlesource.com/62152
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/65479
Run-TryBot: Russ Cox <rsc@golang.org>
This is terrible but much simpler, cleaner, and more effective
than all the alternatives I have come up with.
Lots of code assumes that reflect.DeepEqual is meaningful
on rsa.PublicKey etc, because previously they consisted only of
exported meaningful fields.
Worse, there exists code that assumes asn1.Marshal can be
passed an rsa.PublicKey, because that struct has historically
matched exactly the form that would be needed to produce
the official ASN.1 DER encoding of an RSA public key.
Instead of tracking down and fixing all of that code
(and probably more), we can limit the BoringCrypto-induced
damage by ensliting the compiler to hide the new field
from reflection. Then nothing can get at it and nothing can
be disrupted by it.
Kill two birds with one cannon ball.
I'm very sorry.
Change-Id: I0ca4d6047c7e98f880cbb81904048c1952e278cc
Reviewed-on: https://go-review.googlesource.com/60271
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/65478
Run-TryBot: Russ Cox <rsc@golang.org>
Test is in a separate CL for easier cherry-picking to master branch.
Change-Id: Ia4a9032892d2896332010fe18a3216f8c4a58d1c
Reviewed-on: https://go-review.googlesource.com/59770
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/65477
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
The override was not commented and was in the wrong file.
Change-Id: I739db561acff6d91b0f3559c8bb45437f11c0b04
Reviewed-on: https://go-review.googlesource.com/59250
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/65476
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
This includes the new tags to fix non-cgo builds.
Change-Id: Id2a0dbcf2a229b9e2c0bc35a51312ac1031ce651
Reviewed-on: https://go-review.googlesource.com/58770
Reviewed-by: Adam Langley <agl@golang.org>
The DWARF code is mishandling the case when the host object files
define multiple (distinct) symbols with the same name. They are mapped
to the same DWARF debug symbol, which then appears on the dwarfp
list multiple times, which then breaks the code that processes the list.
Detect duplicates and skip them, because that's trivial, instead of fixing
the underlying problem.
See #21566.
Change-Id: Ib5a34c891d7c15f4c7bb6239d8f31a1ec767b8bc
Reviewed-on: https://go-review.googlesource.com/57943
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/58070
Reviewed-by: Adam Langley <agl@golang.org>
Change-Id: Ia4458090118c4391a73cf1ae65bc8d187f03eca0
Reviewed-on: https://go-review.googlesource.com/58250
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Using atomic.Value causes vet errors in code copying
PublicKey or PrivateKey structures. I don't think the errors
are accurate, but it's easier to work around them than
to change vet or change atomic.Value.
See #21504.
Change-Id: I3a3435c1fc664cc5166c81674f6f7c58dab35f21
Reviewed-on: https://go-review.googlesource.com/56671
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/57941
If we substitute a SHA1 implementation where the entirety of the
reading of the buffer is done in assembly (or C called from cgo),
then the race detector cannot observe the race.
Change to crc32 with a fake polynomial, in the hope that it will
always be handled by Go code, not optimized assembly or cgo calls.
Change-Id: I34e90b14ede6bc220ef686f6aef16b8e464b5cde
Reviewed-on: https://go-review.googlesource.com/56510
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/57934
Reviewed-by: Adam Langley <agl@golang.org>
Right now the package doesn't do anything useful, but it will.
This CL is about the machinery for building goboringcrypto_linux_amd64.syso
and then running the self-test and checking FIPS_mode from Go init.
Change-Id: I4ec0f5efaa88ccfb506b9818d24a7f1cbcc5a7d6
Reviewed-on: https://go-review.googlesource.com/55472
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-on: https://go-review.googlesource.com/57932
This includes the patch for systems that build PIE executables by
defaul
Updates #20276.
Change-Id: Iecf8dfcf11bc18d397b8075559c37e3610f825cb
Reviewed-on: https://go-review.googlesource.com/44470
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/44491
Reviewed-by: Chris Broadfoot <cbro@golang.org>
We shouldn't move a spill to a loop exit where the spill itself
is dead. The stack location assigned to the spill might already
be reused by another spill at this point.
The case we previously handled incorrectly is the one where the value
being spilled is still live, but the spill itself is dead.
Fixes#20472
Patching directly on the release branch because the spill moving code has
already been rewritten for 1.9. (And it doesn't have this bug.)
Change-Id: I26c5273dafd98d66ec448750073c2b354ef89ad6
Reviewed-on: https://go-review.googlesource.com/44033
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
This is a redo of CL 41076 backported to the 1.8 release branch.
There were major conflicts, so I had to basically rewrite it again
from scratch. The way Progs are allocated changed. Liveness analysis
and Prog generation got reordered. Liveness analysis changed from
running on gc.BasicBlock to ssa.Block. All that makes the logic quite
a bit different.
Please review carefully.
From CL 41076:
At VARKILLs, zero a variable if it is ambiguously live.
After the VARKILL anything this variable references
might be collected. If it were to become live again later,
the GC will see references to already-collected objects.
We don't know a variable is ambiguously live until very
late in compilation (after lowering, register allocation, ...),
so it is hard to generate the code in an arch-independent way.
We also have to be careful not to clobber any registers.
Fortunately, this almost never happens so performance is ~irrelevant.
There are only 2 instances where this triggers in the stdlib.
Fixes#20029
Change-Id: Ibb757eec58ee07f40df5e561b19d315684dc4bda
Reviewed-on: https://go-review.googlesource.com/43998
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Commit 4dcba023c6 replaced select with pselect6 on linux/amd64 and
linux/arm, but it turns out the Android emulator uses linux/386. This
makes the equivalent change there, too.
Fixes#20409 more.
Change-Id: If542d6ade06309aab8758d5f5f6edec201ca7670
Reviewed-on: https://go-review.googlesource.com/44011
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit ecad34a40ea390ddf5ba2da8f3c3f2c5f15297c8)
Reviewed-on: https://go-review.googlesource.com/44002
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Android O black-lists the select system call because its libc, Bionic,
does not use this system call. Replace our use of select with pselect6
(which is allowed) on the platforms that support targeting Android.
linux/arm64 already uses pselect6 because there is no select on arm64,
so only linux/amd64 and linux/arm need changing. pselect6 has been
available since Linux 2.6.16, which is before Go's minimum
requirement.
Fixes#20409.
Change-Id: Ic526b5b259a9e01d2f145a1f4d2e76e8c49ce809
Reviewed-on: https://go-review.googlesource.com/43641
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit 4dcba023c6)
Reviewed-on: https://go-review.googlesource.com/44001
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
On s390x unsigned integer comparisons with immediates require the immediate
to be an unsigned 32-bit integer. The rule was checking that the immediate
was a signed 32-bit integer.
This CL also adds a test for comparisons that could be turned into compare
with immediate or equivalent instructions (depending on architecture and
optimizations applied).
Cherry-pick of CL 40433 and CL 40873.
Fixes#19940.
Reviewed-on: https://go-review.googlesource.com/40931
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Change-Id: I3daaeaa40d7637bd4421e6b8d37ea4ffd74448ce
Reviewed-on: https://go-review.googlesource.com/43994
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Cherry-pick of CL 43870.
If mheap.sysAlloc doesn't have room in the heap arena for an
allocation, it will attempt to map more address space with sysReserve.
sysReserve is given a hint, but can return any unused address range.
Currently, mheap.sysAlloc incorrectly assumes the returned region will
never fall between arena_start and arena_used. If it does,
mheap.sysAlloc will blindly accept the new region as the new
arena_used and arena_end, causing these to decrease and make it so any
Go heap above the new arena_used is no longer considered part of the
Go heap. This assumption *used to be* safe because we had all memory
between arena_start and arena_used mapped, but when we switched to an
arena_start of 0 on 32-bit, it became no longer safe.
Most likely, we've only recently seen this bug occur because we
usually start arena_used just above the binary, which is low in the
address space. Hence, the kernel is very unlikely to give us a region
before arena_used.
Since mheap.sysAlloc is a linear allocator, there's not much we can do
to handle this well. Hence, we fix this problem by simply rejecting
the new region if it isn't after arena_end. In this case, we'll take
the fall-back path and mmap a small region at any address just for the
requested memory.
Fixes#20259.
Change-Id: Ib72e8cd621545002d595c7cade1e817cfe3e5b1e
Reviewed-on: https://go-review.googlesource.com/43954
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Cherry-pick of CL 43294.
Tuple ops are weird. They are essentially a pair of ops,
one which consumes a mem and one which generates a mem (the Select1).
The schedule pass didn't handle these quite right.
Fix the scheduler to include both parts of the paired op in
the store chain. That makes sure that loads are correctly ordered
with respect to the first of the pair.
Add a check for the ssacheck builder, that there is only one
live store at a time. I thought we already had such a check, but
apparently not...
Fixes#20335
Change-Id: I59eb3446a329100af38d22820b1ca2190ca46a78
Reviewed-on: https://go-review.googlesource.com/43411
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Cherry-pick of CL 43311.
runtime.gchelper depends on the non-atomic load of work.ndone
happening strictly before the atomic add of work.nwait. Until very
recently (commit 978af9c2db, fixing #20334), the compiler reordered
these operations. This created a race since work.ndone can change as
soon as work.nwait is equal to work.ndone. If that happened, more than
one gchelper could attempt to wake up the work.alldone note, causing a
"double wakeup" panic.
This was fixed in the compiler, but to make this code less subtle,
make the load of work.ndone atomic. This clearly forces the order of
these operations, ensuring the race doesn't happen.
Fixes#19305 (though really 978af9c2db fixed it).
Change-Id: Ieb1a84e1e5044c33ac612c8a5ab6297e7db4c57d
Reviewed-on: https://go-review.googlesource.com/43412
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Applies https://golang.org/cl/42139 to the go1.8 release branch.
Also correct two minor issues detected with go vet.
Fixes#20217
Change-Id: I2c41af9497493598fbcfc140439b4e25b9bb7e72
Reviewed-on: https://go-review.googlesource.com/42532
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Russ Cox
Chris Broadfoot
Keith Randall
Austin Clements
Todd Neal
Brad Fitzpatrick
Michael Munday
Daniel Theophanes