Rework the test data story
We originally embedded test data because of deficiencies in Android's
build. Android had no way to specify test data with tests. That has
since been resolved, and the embedding mechanism has gotten unwieldy.
This unifies pki_test and crypto_test's test data story, and does so in
a way that all tests can participate in. (We can now use FileTest in
decrepit_test.)
Update-Note: This will require some tweaks to downstream builds. We no
longer emit an (unwieldy) crypto_test_data.cc file. Instead, tests will
expect test data be available at the current working directory. This can
be overridden with the BORINGSSL_TEST_DATA_ROOT environment variable.
Callers with more complex needs can build with
BORINGSSL_CUSTOM_GET_TEST_DATA and then link in an alternate
implementation of this function.
On the off chance some project needs it, I've kept the
embed_test_data.go script around for now, but I expect we can delete it
in the future.
Fixed: 681
Change-Id: If181ce043e1eea3148838f1bb4db9ee4bfda0d08
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/67295
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 35fb567..1cff48f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -490,26 +490,6 @@
# themselves as dependencies next to the target definition.
add_custom_target(all_tests)
-# On Windows, CRYPTO_TEST_DATA is too long to fit in command-line limits.
-# TODO(davidben): CMake 3.12 has a list(JOIN) command. Use that when we've
-# updated the minimum version.
-set(EMBED_TEST_DATA_ARGS "")
-foreach(arg ${CRYPTO_TEST_DATA})
- set(EMBED_TEST_DATA_ARGS "${EMBED_TEST_DATA_ARGS}${arg}\n")
-endforeach()
-file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/embed_test_data_args.txt"
- "${EMBED_TEST_DATA_ARGS}")
-
-add_custom_command(
- OUTPUT crypto_test_data.cc
- COMMAND ${GO_EXECUTABLE} run util/embed_test_data.go -file-list
- "${CMAKE_CURRENT_BINARY_DIR}/embed_test_data_args.txt" >
- "${CMAKE_CURRENT_BINARY_DIR}/crypto_test_data.cc"
- DEPENDS util/embed_test_data.go ${CRYPTO_TEST_DATA}
- WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})
-
-add_library(crypto_test_data OBJECT crypto_test_data.cc)
-
add_subdirectory(crypto)
add_subdirectory(ssl/test)
add_subdirectory(util/fipstools)
@@ -550,7 +530,7 @@
target_link_libraries(urandom_test test_support_lib boringssl_gtest crypto)
add_dependencies(all_tests urandom_test)
-add_executable(crypto_test ${CRYPTO_TEST_SOURCES} $<TARGET_OBJECTS:crypto_test_data>)
+add_executable(crypto_test ${CRYPTO_TEST_SOURCES})
target_link_libraries(crypto_test test_support_lib boringssl_gtest crypto)
add_dependencies(all_tests crypto_test)
diff --git a/build.json b/build.json
index 8c1de1e..0463e88 100644
--- a/build.json
+++ b/build.json
@@ -202,7 +202,9 @@
"srcs": [
"crypto/test/abi_test.cc",
"crypto/test/file_test.cc",
+ "crypto/test/file_test_gtest.cc",
"crypto/test/file_util.cc",
+ "crypto/test/test_data.cc",
"crypto/test/test_util.cc",
"crypto/test/wycheproof_util.cc"
],
@@ -285,10 +287,6 @@
"crypto/siphash/siphash_test.cc",
"crypto/spx/spx_test.cc",
"crypto/thread_test.cc",
- // TODO(crbug.com/boringssl/542): This should be in test_support, so
- // that all tests can use it. But it depends on GetTestData, which
- // is not currently usable outside of crypto_test.
- "crypto/test/file_test_gtest.cc",
"crypto/test/gtest_main.cc",
"crypto/trust_token/trust_token_test.cc",
"crypto/x509/tab_test.cc",
diff --git a/crypto/pkcs8/pkcs12_test.cc b/crypto/pkcs8/pkcs12_test.cc
index 459339d..a210089 100644
--- a/crypto/pkcs8/pkcs12_test.cc
+++ b/crypto/pkcs8/pkcs12_test.cc
@@ -25,11 +25,10 @@
#include <openssl/stack.h>
#include <openssl/x509.h>
+#include "../test/test_data.h"
#include "../test/test_util.h"
-std::string GetTestData(const char *path);
-
// kPassword is the password shared by most of the sample PKCS#12 files.
static const char kPassword[] = "foo";
diff --git a/crypto/test/file_test_gtest.cc b/crypto/test/file_test_gtest.cc
index 2cb0896..b1d3556 100644
--- a/crypto/test/file_test_gtest.cc
+++ b/crypto/test/file_test_gtest.cc
@@ -25,8 +25,8 @@
#include <openssl/err.h>
+#include "test_data.h"
-std::string GetTestData(const char *path);
class StringLineReader : public FileTest::LineReader {
public:
diff --git a/crypto/test/test_data.cc b/crypto/test/test_data.cc
new file mode 100644
index 0000000..9fe2aaa
--- /dev/null
+++ b/crypto/test/test_data.cc
@@ -0,0 +1,51 @@
+/* Copyright (c) 2024, Google Inc.
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
+ * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
+ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
+
+#include "test_data.h"
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "file_util.h"
+
+#if !defined(BORINGSSL_CUSTOM_GET_TEST_DATA)
+std::string GetTestData(const char *path) {
+ const char *root = getenv("BORINGSSL_TEST_DATA_ROOT");
+ root = root != nullptr ? root : ".";
+
+ std::string full_path = root;
+ full_path.push_back('/');
+ full_path.append(path);
+
+ ScopedFILE file(fopen(full_path.c_str(), "rb"));
+ if (file == nullptr) {
+ fprintf(stderr, "Could not open '%s'.\n", full_path.c_str());
+ abort();
+ }
+
+ std::string ret;
+ for (;;) {
+ char buf[512];
+ size_t n = fread(buf, 1, sizeof(buf), file.get());
+ if (n == 0) {
+ if (feof(file.get())) {
+ return ret;
+ }
+ fprintf(stderr, "Error reading from '%s'.\n", full_path.c_str());
+ abort();
+ }
+ ret.append(buf, n);
+ }
+}
+#endif // !BORINGSSL_CUSTOM_GET_TEST_DATA
diff --git a/crypto/test/test_data.h b/crypto/test/test_data.h
new file mode 100644
index 0000000..b3f4b6a
--- /dev/null
+++ b/crypto/test/test_data.h
@@ -0,0 +1,31 @@
+/* Copyright (c) 2024, Google Inc.
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
+ * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
+ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
+ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
+
+#ifndef OPENSSL_HEADER_CRYPTO_TEST_TEST_DATA_H
+#define OPENSSL_HEADER_CRYPTO_TEST_TEST_DATA_H
+
+#include <string>
+
+// GetTestData returns the test data for |path|, or aborts on error. |path|
+// must be a slash-separated path, relative to the BoringSSL source tree. By
+// default, this is implemented by reading from the filesystem, relative to
+// the BORINGSSL_TEST_DATA_ROOT environment variable, or the current working
+// directory if unset.
+//
+// Callers with more complex needs can build with
+// BORINGSSL_CUSTOM_GET_TEST_DATA and then link in an alternate implementation
+// of this function.
+std::string GetTestData(const char *path);
+
+#endif // OPENSSL_HEADER_CRYPTO_TEST_TEST_DATA_H
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 4559b22..ffbcf7b 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -37,6 +37,7 @@
#include "internal.h"
#include "../internal.h"
#include "../test/file_util.h"
+#include "../test/test_data.h"
#include "../test/test_util.h"
#if defined(OPENSSL_THREADS)
@@ -44,8 +45,6 @@
#endif
-std::string GetTestData(const char *path);
-
static const char kCrossSigningRootPEM[] = R"(
-----BEGIN CERTIFICATE-----
MIICcTCCAdqgAwIBAgIIagJHiPvE0MowDQYJKoZIhvcNAQELBQAwPDEaMBgGA1UE
diff --git a/gen/sources.cmake b/gen/sources.cmake
index 9153d77..e88cb24 100644
--- a/gen/sources.cmake
+++ b/gen/sources.cmake
@@ -277,7 +277,6 @@
crypto/siphash/siphash_test.cc
crypto/spx/spx_test.cc
crypto/stack/stack_test.cc
- crypto/test/file_test_gtest.cc
crypto/test/gtest_main.cc
crypto/thread_test.cc
crypto/trust_token/trust_token_test.cc
@@ -2100,7 +2099,9 @@
crypto/test/abi_test.cc
crypto/test/file_test.cc
+ crypto/test/file_test_gtest.cc
crypto/test/file_util.cc
+ crypto/test/test_data.cc
crypto/test/test_util.cc
crypto/test/wycheproof_util.cc
)
diff --git a/gen/sources.json b/gen/sources.json
index 90b6247..d00a910 100644
--- a/gen/sources.json
+++ b/gen/sources.json
@@ -249,7 +249,6 @@
"crypto/siphash/siphash_test.cc",
"crypto/spx/spx_test.cc",
"crypto/stack/stack_test.cc",
- "crypto/test/file_test_gtest.cc",
"crypto/test/gtest_main.cc",
"crypto/thread_test.cc",
"crypto/trust_token/trust_token_test.cc",
@@ -2059,7 +2058,9 @@
"srcs": [
"crypto/test/abi_test.cc",
"crypto/test/file_test.cc",
+ "crypto/test/file_test_gtest.cc",
"crypto/test/file_util.cc",
+ "crypto/test/test_data.cc",
"crypto/test/test_util.cc",
"crypto/test/wycheproof_util.cc"
],
diff --git a/pki/test_helpers.cc b/pki/test_helpers.cc
index b6712af..490fba5 100644
--- a/pki/test_helpers.cc
+++ b/pki/test_helpers.cc
@@ -12,9 +12,12 @@
#include <string_view>
#include <gtest/gtest.h>
+
#include <openssl/bytestring.h>
#include <openssl/mem.h>
#include <openssl/pool.h>
+
+#include "../crypto/test/test_data.h"
#include "cert_error_params.h"
#include "cert_errors.h"
#include "parser.h"
@@ -90,41 +93,6 @@
return out;
}
-bool ReadFileToString(const std::string &path, std::string *out) {
- std::ifstream file(path, std::ios::binary);
- file.unsetf(std::ios::skipws);
-
- file.seekg(0, std::ios::end);
- if (file.tellg() == -1) {
- return false;
- }
- out->reserve(file.tellg());
- file.seekg(0, std::ios::beg);
-
- out->assign(std::istreambuf_iterator<char>(file),
- std::istreambuf_iterator<char>());
-
- return true;
-}
-
-std::string AppendComponent(const std::string &path,
- const std::string &component) {
- // Append a path component to a path. Use the \ separator if this appears to
- // be a Windows path, otherwise the Unix one.
- if (path.find(":\\") != std::string::npos) {
- return path + "\\" + component;
- }
- return path + "/" + component;
-}
-
-std::string GetTestRoot(void) {
- // We expect our test data to live in "pki" underneath a
- // test root directory, or in the current directry.
- char *root_from_env = getenv("BORINGSSL_TEST_DATA_ROOT");
- std::string root = root_from_env ? root_from_env : ".";
- return AppendComponent(root, "pki");
-}
-
} // namespace
namespace der {
@@ -450,18 +418,7 @@
}
std::string ReadTestFileToString(const std::string &file_path_ascii) {
- // Compute the full path, relative to the src/ directory.
- std::string src_root = GetTestRoot();
- std::string filepath = AppendComponent(src_root, file_path_ascii);
-
- // Read the full contents of the file.
- std::string file_data;
- if (!ReadFileToString(filepath, &file_data)) {
- ADD_FAILURE() << "Couldn't read file: " << filepath;
- return std::string();
- }
-
- return file_data;
+ return GetTestData(("pki/" + file_path_ascii).c_str());
}
void VerifyCertPathErrors(const std::string &expected_errors_str,
diff --git a/util/generate_build_files.py b/util/generate_build_files.py
index 9e625c8..a54a614 100644
--- a/util/generate_build_files.py
+++ b/util/generate_build_files.py
@@ -24,7 +24,6 @@
PREFIX = None
-EMBED_TEST_DATA = True
def PathOf(x):
@@ -80,12 +79,12 @@
non_bcm_asm = self.FilterBcmAsm(files['crypto_asm'], False)
bcm_asm = self.FilterBcmAsm(files['crypto_asm'], True)
- self.PrintDefaults(blueprint, 'libcrypto_sources', non_bcm_c_files, non_bcm_asm)
- self.PrintDefaults(blueprint, 'libcrypto_bcm_sources', bcm_c_files, bcm_asm)
+ self.PrintDefaults(blueprint, 'libcrypto_sources', non_bcm_c_files, asm_files=non_bcm_asm)
+ self.PrintDefaults(blueprint, 'libcrypto_bcm_sources', bcm_c_files, asm_files=bcm_asm)
self.PrintDefaults(blueprint, 'libssl_sources', files['ssl'])
self.PrintDefaults(blueprint, 'bssl_sources', files['tool'])
self.PrintDefaults(blueprint, 'boringssl_test_support_sources', files['test_support'])
- self.PrintDefaults(blueprint, 'boringssl_crypto_test_sources', files['crypto_test'])
+ self.PrintDefaults(blueprint, 'boringssl_crypto_test_sources', files['crypto_test'], data=files['crypto_test_data'])
self.PrintDefaults(blueprint, 'boringssl_ssl_test_sources', files['ssl_test'])
self.PrintDefaults(blueprint, 'libpki_sources', files['pki'])
@@ -97,7 +96,7 @@
self.PrintVariableSection(makefile, 'crypto_sources_asm',
files['crypto_asm'])
- def PrintDefaults(self, blueprint, name, files, asm_files=[]):
+ def PrintDefaults(self, blueprint, name, files, asm_files=[], data=[]):
"""Print a cc_defaults section from a list of C files and optionally assembly outputs"""
if asm_files:
blueprint.write('\n')
@@ -113,6 +112,11 @@
for f in sorted(files):
blueprint.write(' "%s",\n' % f)
blueprint.write(' ],\n')
+ if data:
+ blueprint.write(' data: [\n')
+ for f in sorted(data):
+ blueprint.write(' "%s",\n' % f)
+ blueprint.write(' ],\n')
if asm_files:
blueprint.write(' target: {\n')
@@ -655,19 +659,6 @@
FindHeaderFiles(os.path.join('src', 'crypto', 'test'), AllFiles) +
FindHeaderFiles(os.path.join('src', 'ssl', 'test'), NoTestRunnerFiles))
- crypto_test_files = []
- if EMBED_TEST_DATA:
- # Generate crypto_test_data.cc
- with open('crypto_test_data.cc', 'w+') as out:
- subprocess.check_call(
- ['go', 'run', 'util/embed_test_data.go'] + sources['crypto_test']['data'],
- cwd='src',
- stdout=out)
- crypto_test_files.append('crypto_test_data.cc')
-
- crypto_test_files += PrefixWithSrc(sources['crypto_test']['srcs'])
- crypto_test_files.sort()
-
fuzz_c_files = FindCFiles(os.path.join('src', 'fuzz'), NoTests)
ssl_h_files = FindHeaderFiles(os.path.join('src', 'include', 'openssl'),
@@ -700,7 +691,7 @@
'crypto_nasm': PrefixWithSrc(crypto_nasm),
'crypto_headers': crypto_h_files,
'crypto_internal_headers': crypto_internal_h_files,
- 'crypto_test': crypto_test_files,
+ 'crypto_test': PrefixWithSrc(sources['crypto_test']['srcs']),
'crypto_test_data': PrefixWithSrc(sources['crypto_test']['data']),
'fips_fragments': fips_fragments,
'fuzz': fuzz_c_files,
@@ -744,13 +735,8 @@
'|'.join(sorted(ALL_PLATFORMS.keys())))
parser.add_option('--prefix', dest='prefix',
help='For Bazel, prepend argument to all source files')
- parser.add_option(
- '--embed_test_data', type='choice', dest='embed_test_data',
- action='store', default="true", choices=["true", "false"],
- help='For Bazel or GN, don\'t embed data files in crypto_test_data.cc')
options, args = parser.parse_args(sys.argv[1:])
PREFIX = options.prefix
- EMBED_TEST_DATA = (options.embed_test_data == "true")
if not args:
parser.print_help()
