Referencing a variable in a closure captures it by _address_. So
referencing a loop variable can go horribly wrong:
https://go.dev/play/p/f2ivPAIN_bG
This is accepted as essentially a bug by Go and will be fixed in a
future release (https://github.com/golang/go/wiki/LoopvarExperiment).
But, for now at least, work around it.
Our tests trim the ACVP inputs to only have a single test case per group
in many cases, which hides most of this issue from tests. When we run
run full ACVP sets, our modulewrapper is seemingly fast enough not to
notice there either. But I've updated one of the tests here by
duplicating a test case enough that it catches this a meaningful amount
of the time.
Change-Id: I8216c00f67636ab7dad927eae4b49ae45ae3cf31
Bug: 646
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62965
Reviewed-by: David Benjamin <davidben@google.com>
Auto-Submit: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This reverts 1e2f169663. Bazel 6.3 has
since been released, which includes a fix for
https://github.com/bazelbuild/bazel/issues/15073. Envoy and gRPC have
both since updated to this Bazel version. The policies in
https://opensource.google/documentation/policies/cplusplus-support#build_systems
also imply a minimum Bazel version of 6.3.2.
I'm thinking we let this bake for a little while, to catch any
unexpected issues, and then, if it sticks, we try to go ahead and
require C11 across the board.
Update-Note: If using Bazel with MSVC, and the build fails with
something like "Command line error D8016 : '/std:c++20' and '/std:c11'
command-line options are incompatible", you are likely running into the
above Bazel bug. Update to Bazel 6.3 or later.
Bug: 623, 624
Change-Id: I8baa99392ca47bc7580bc2930e7f4b16beced91e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62905
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
sha512-armv8.pl and sha512-x86_64.pl implement both SHA-256 and SHA-512
and select which to emit by looking for "512" in the output path.
This can result in a false positive if the output path happens to
contain "512" in it. When the build uses relative paths, it's fine, but
this seems needlessly fragile. If we're generate into a temporary file,
there's a small but non-negligible probability that the path has a
"512" in it.
Instead, give those scripts three arguments: flavor hash output, so the
selection is independent of the output file name.
Bug: 542
Change-Id: Idf256abed1c07003034d3eb4544552125e3289e5
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62325
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
I think there are a couple projects remaining that still use the old
lists here, but they're in repositories we don't spend as much time in,
and it should be straightforward for them to update when they get here.
Removing these should put us in a good place to check in pre-generated
asm lists. While I'm here, fix a few typos in TODOs I previously added.
Update-Note: If you're one of those projects and have trouble switching
to the new lists, let us know.
Bug: 542
Change-Id: I57559bafc85eceacc7a237e2f29db6eaf492a8cb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62186
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
e4acd6cb56 enabled assembly for more
platforms on Android, but the way Android's FIPS build is set up, we
currently require ld -r to work on any platform with assembly. See
b/294399371.
This should be fixable with more time spent on the Android build (and
possibly missing features added to Soong, as Soong is quite limited),
but as we've never had assembly working here, just restore the old state
of things.
Change-Id: I8f4e66979a003a5692389ef7e127c8d9f1630773
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/62165
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This doesn't remove the old ones yet as we'll need a corresponding
downstream change. This does all of them but Soong. I think, for Soong,
we can apply the change unilaterally in the source list, but I'm holding
off on testing that until I've gotten Android updated to a more recent
BoringSSL.
Bug: 542
Change-Id: I30921ac251f76e3ecb50df9ff757fec4031e7dbc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61528
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Update-Note: I believe all GN and Bazel consumers have been migrated to
the new style of asm source list, so this change should be a no-op. If
any remain that still need the old ones, either migrate them to
crypto_sources_asm and crypto_sources_nasm, or let us know.
Bug: 542
Change-Id: I57052331ae56a081117b1a2df31f828943a6d13b
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61527
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
As part of this, align the generated and standalone builds in how
crypto/test/gtest_main.cc is added. Since not all test targets
(urandom_test) include gtest_main.cc, and the generated one just
manually adds it to the file lists, just put it into the file lists
ahead of time. That way we don't need to synchronize the dependency
information with the generated build.
This also aligns the generated build with
https://boringssl-review.googlesource.com/c/boringssl/+/56567 to put
files like crypto/test/abi_test.cc and crypto/test/file_test_gtest.cc in
the test_support library.
Update-Note: If something odd happens with the test_support library in
downstream builds, this CL is probably to blame.
Bug: 542
Change-Id: I235e0ccd0432f4b380a92b265ede35eb8a6a6e36
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61288
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
CMake and the generate builds now broadly share a source of truth for
the test files.
Update-Note: In the standalone CMake build, build/crypto/crypto_test is
now build/crypto_test, etc. For now, the build still copies the outputs
to the subdirectories (it's cheap and avoids some workflow turbulence),
but I'm thinking we keep that for six months or so and then remove it.
Bug: 542
Change-Id: I8f97e1fcedea1375d48567dfd2da01a6e66ec4e8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61286
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
This reverts commit 28e4a1b838. Bazel
broke --cxxopt on Windows in
https://github.com/bazelbuild/bazel/issues/15073, which means projects
enabling, say, C++20 with --cxxopt=/std:c++20 are silently passing
/std:c++20 to our C files.
This is already a problem, but MSVC is smart enough to silently ignore
the flag when building C. However, MSVC will report an error if you then
pass /std:c++20 /std:c11 into the same command. It seems that check is
not aware of this ignoring behavior.
Ultimately, this is a Bazel bug, and one that makes the broken versions
of Bazel unsuitable for use with C. This was fixed in Bazel in
https://github.com/bazelbuild/bazel/pull/18119 and backported to the
upcoming Bazel 6.3.0 release in
https://github.com/bazelbuild/bazel/pull/18552
Temporarily revert the change. When Bazel 6.3.0 is released, we'll put
this back and require Windows users use a functioning version of Bazel.
Bug: 624
Fixed: 623
Change-Id: I68d9b2ed8751b4cf5dc7f42f8c1fbd42a97d6ca2
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61365
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
This removes a place where we make hard-coded assumes about test names.
Also it shards pki_test, as that test suite is large enough to benefit
from it.
Change-Id: I392254b73a2df2f022ccf13508552372c103bff7
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61285
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This achieves much of the same goals as
https://boringssl-review.googlesource.com/c/boringssl/+/61245, but
reuses the existing source.cmake parser in generate_build_files.py.
There are two practical differences here:
First, CMake knows that, if it sees include(sources.cmake), it should
add sources.cmake as a dependency of build.ninja, and re-run cmake if
source.cmake changes. It does not know this for file(STRINGS). This can
be fixed with CMAKE_CONFIGURE_DEPENDS, but since we already have this,
may as well use it.
Second, sources.cmake wants paths rooted at the top-level BoringSSL
source directory, which means we want to define the targets at the top
level, rather than in subdirectories. While it will make the top-level
CMakeLists.txt larger, I think this is a good move:
- ./build/crypto_test --gtest_filter=... is just less annoying to type
than ./build/crypto/crypto_test --gtest_filter=...
- It will (eventually) mean libcrypto.a lives at build/libcrypto.a
instead of build/crypto/libcrypto.a. This means we can pass a single
-L parameter when building things against BoringSSL, without relying
on the install target.
- It aligns with the generated CMake build. I would like, long-term, to
stop maintaining multiple CMake builds, and the generated one seems to
have a better convention. And one that will be more disruptive to
others if we change it.
- Top-level-rooted paths are more convenient for generate_build_files.py
to work with, because that's what we want to output.
As I get further along in 542, I expect I'll move this once again,
possibly to some JSON file, because I'll need my new pregenerate tool to
parse it (and we'll no longer be constrained by what's easy to consume
in CMake), but use this for now.
Bug: 542
Change-Id: I1e8b894057264da5d7624a1fbb92f9e1198ea38e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61266
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
We require MSVC 2019 now, which has a /std:c11 flag. Enable it to match
the CMake build and remove a blocker for requiring C11 unconditionally.
(This select branch is also used by clang-cl. I had meant to figure out
the @bazel_tools business as part of this, but it turns out clang-cl
works better with the MSVC flags than the GCC ones anyway. -Wall in
clang-cl is like MSVC's /Wall and actually means all warnings. Ideally
we'd still condition this on the compiler, in case anyone uses MinGW,
but we can figure that out later.)
Tested with bazelisk build :all and
bazelisk build --compiler=clang-cl :all on Windows.
Change-Id: I4559789a221071eef39f9d34929f0e9c5994119e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/61127
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Initially this leaves the canonical source in chrome, Additions
and fillins are committed directly, the chrome files are coverted
using the IMPORT script run from the pki directory for the moment.
The intention here is to continue frequent automatic conversion
(and avoid wholesale cosmetic changes in here for now) until
chrome converts to use these files in place of it's versions.
At that point these will become the definiative files, and the
IMPORT script can be tossed out.
A middle step along the way will be to change google3's verify.cc
in third_party/chromium_certificate_verifier to use this instead
of it's own extracted copy.
Status (and what is not done yet) being roughly tracked in README.md
Bug: chromium:1322914
Change-Id: Ibdb5479bc68985fa61ce6b10f98f31f6b3a7cbdf
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60285
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>
The compiler already knows that .S means assembler and should define
__ASSEMBLER__ for us. At least on the CQ, it seems this is not
necessary, and some builds are showing a warning about redefining
__ASSEMBLER__, which may be caused by this.
Change-Id: I390bfcb50cefef42df72cd61ead55159b1838771
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60785
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
This fixes the generated Bazel build. Bazel is strict about having all
dependencies declared, which includes files that are #included into
other files. (It also is not particularly pleased about textual
headers and wants them declared in a separate place.)
The new fiat curve25519 assembly is currently split into a BoringSSL
half, and a more generic fiat half. For now, just move the BoringSSL
customizations directly into the fiat half. This isn't ideal, as we'd,
long term, like something where the fiat code can be made standalone.
But, to fix the build, just patch in the changes now and we can ponder
how to do this better later. (Build tools and conventions for assembly
are much less clear than C, sadly.)
Also add the .note.GNU-stack bit at the end.
Change-Id: I04aa733eabf8562dba42dee63a8fd25c86a59db9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60566
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
This fixes one of the two build issues triggered by the curve25519
assembly. The new curve25519 assembly is one file that works for both
ELF and Apple targets. This is handy, but breaks the naive concatenation
logic for making the generated file lists. Fix the generation to
deduplicate first.
Convincing Starlark in Bazel to deduplicate is annoyingly difficult, so
we'll just generate both lists. This results in some duplication in the
generated files, but they're generated so this is fine. Hopefully we
can, in time, just remove the per-platform lists when everyone is
migrated over. To that end, this CL adds the combined output to GN so I
can migrate Chromium.
This CL is not sufficient to unbreak the Bazel build. The next change is
also needed.
Bug: 542
Change-Id: Ibe76ff557fc43f7b4d984ccdf298f13c20f3b50c
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60565
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Also teach update_clang.py how to download Chromium's Clang on macOS. We
don't currently use it in CI, but I've patched this in enough times for
local testing, that having it seems convenient.
This picks up -fsanitize=function, though it wouldn't have caught the
qsort issue anyway. I'm guessing the libc must be built with UBSan too.
Change-Id: I8c396a10b32e69fe7013283b1bb4320bc35756b6
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60547
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
The Close() method of the middle often wasn't getting called because
`os.Exit(0)` was used in some places. Once that's fixed, it's clear that
the queue of pending reads needed to be closed before waiting for the
reader goroutine to finish. Lastly, don't bother trying to record the
error that the reader saw: just panic the process if the modulewrapper
dies during processing.
Change-Id: Icf077cefd0ace2ef721a493f99fede6269531257
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60045
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: Adam Langley <agl@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Satisfy a go lint. As of Go 1.18, any is the preferred spelling of
interface{}.
Also remove an instance of redundant types in
util/fipstools/acvp/acvptool/acvp.go because my editor warned about it.
(A []map[string]any{map[string]any{...}, map[string]any{...}} literal
can omit the inner copy of the type because it's implicit from the outer
one.)
Change-Id: I2251b2285c16c19bc779fa41d1011f7fa1392563
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59465
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
This can still be run like go run ../../util/convert_wycheproof. This is
part of an attempt to reland 54b04fdc21,
which ran into an issue with internal tooling that could not handle
standalone Go files.
Since the only such target we actually needed to run in that repository
is convert_wycheproof, just promote it into its own package. Then we can
stop trying to import util.
Change-Id: I6237777dad09e5c81ad961816ce14a287ab2d46a
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59185
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
(This change breaks running on Android. But it's close to what we used
for non-Android FIPS testing so is useful to have on the record.)
This change adds testing of the run-time break tests: the pair-wise
consistency tests and the RNG test.
It also switches to using two test_fips binaries: an unmodified one for
showing a clean up and testing the integrity test, and a test_fips_break
which makes integrity test failures non-fatal (for KAT testing) and
which allows the run-time tests to be triggered.
Change-Id: Id2787723059cfb17cc2d22013ad66b985ef86701
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53885
Reviewed-by: David Benjamin <davidben@google.com>
Auto-Submit: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
766C6164