Add build-time checks of AIDL API hashes
All frozen AIDL APIs are required to have a hash, and these are checked
for accuracy at build time. This ensures that these interfaces aren't
changed, since we are relying on their stability over time.
Bug: 150154330
Test: atest CtsNdkBinderTestCases, freeze a new interface and verify
Change-Id: Iff9ce5b8022ebd587ee451eb45d070e500da430a
diff --git a/aidl.cpp b/aidl.cpp
index 083d520..2c9bd2e 100644
--- a/aidl.cpp
+++ b/aidl.cpp
@@ -840,7 +840,7 @@
unique_ptr<CodeWriter> writer =
io_delegate.GetCodeWriter(GetApiDumpPathFor(*type, options));
if (!type->GetPackage().empty()) {
- (*writer) << "package " << type->GetPackage() << ";\n";
+ (*writer) << kPreamble << "package " << type->GetPackage() << ";\n";
}
type->Dump(writer.get());
}
diff --git a/aidl.h b/aidl.h
index 94eaf54..298a6cc 100644
--- a/aidl.h
+++ b/aidl.h
@@ -50,6 +50,26 @@
bool dump_api(const Options& options, const IoDelegate& io_delegate);
bool dump_mappings(const Options& options, const IoDelegate& io_delegate);
+const char kPreamble[] =
+ R"(///////////////////////////////////////////////////////////////////////////////
+// THIS FILE IS IMMUTABLE. DO NOT EDIT IN ANY CASE. //
+///////////////////////////////////////////////////////////////////////////////
+
+// This file is a snapshot of an AIDL interface (or parcelable). Do not try to
+// edit this file. It looks like you are doing that because you have modified
+// an AIDL interface in a backward-incompatible way, e.g., deleting a function
+// from an interface or a field from a parcelable and it broke the build. That
+// breakage is intended.
+//
+// You must not make a backward incompatible changes to the AIDL files built
+// with the aidl_interface module type with versions property set. The module
+// type is used to build AIDL files in a way that they can be used across
+// independently updatable components of the system. If a device is shipped
+// with such a backward incompatible change, it has a high risk of breaking
+// later when a module using the interface is updated, e.g., Mainline modules.
+
+)";
+
const string kGetInterfaceVersion("getInterfaceVersion");
const string kGetInterfaceHash("getInterfaceHash");
diff --git a/aidl_unittest.cpp b/aidl_unittest.cpp
index 9e22453..21ba79c 100644
--- a/aidl_unittest.cpp
+++ b/aidl_unittest.cpp
@@ -928,7 +928,7 @@
ASSERT_TRUE(result);
string actual;
EXPECT_TRUE(io_delegate_.GetWrittenContents("dump/foo/bar/IFoo.aidl", &actual));
- EXPECT_EQ(actual, R"(package foo.bar;
+ EXPECT_EQ(actual, string(kPreamble).append(R"(package foo.bar;
/* @hide */
interface IFoo {
/* @hide */
@@ -940,10 +940,10 @@
const int A = 1;
const String STR = "Hello";
}
-)");
+)"));
EXPECT_TRUE(io_delegate_.GetWrittenContents("dump/foo/bar/Data.aidl", &actual));
- EXPECT_EQ(actual, R"(package foo.bar;
+ EXPECT_EQ(actual, string(kPreamble).append(R"(package foo.bar;
/* @hide */
parcelable Data {
/* @hide */
@@ -956,7 +956,7 @@
/* @hide */
@nullable String[] c;
}
-)");
+)"));
}
TEST_F(AidlTest, ApiDumpWithManualIds) {
@@ -975,13 +975,13 @@
ASSERT_TRUE(result);
string actual;
EXPECT_TRUE(io_delegate_.GetWrittenContents("dump/foo/bar/IFoo.aidl", &actual));
- EXPECT_EQ(actual, R"(package foo.bar;
+ EXPECT_EQ(actual, string(kPreamble).append(R"(package foo.bar;
interface IFoo {
int foo() = 1;
int bar() = 2;
int baz() = 10;
}
-)");
+)"));
}
TEST_F(AidlTest, ApiDumpWithManualIdsOnlyOnSomeMethods) {
diff --git a/build/aidl_api/test-piece-1/1/.hash b/build/aidl_api/test-piece-1/1/.hash
index 3185626..79228df 100644
--- a/build/aidl_api/test-piece-1/1/.hash
+++ b/build/aidl_api/test-piece-1/1/.hash
@@ -1 +1 @@
-db5ac8f6ff15c327e0da41396ba7af9ec21fc48c -
+13e24b2fac6a979971819fba2ab0d6d7c4182122
diff --git a/build/aidl_api/test-piece-1/2/.hash b/build/aidl_api/test-piece-1/2/.hash
index 5bcc485..2ad0e01 100644
--- a/build/aidl_api/test-piece-1/2/.hash
+++ b/build/aidl_api/test-piece-1/2/.hash
@@ -1 +1 @@
-dc2a9292847e43b4360bb183f7491f0e9895eaa9 -
+dc2a9292847e43b4360bb183f7491f0e9895eaa9
diff --git a/build/aidl_api/test-piece-1/3/.hash b/build/aidl_api/test-piece-1/3/.hash
index 6a6f341..58a9b34 100644
--- a/build/aidl_api/test-piece-1/3/.hash
+++ b/build/aidl_api/test-piece-1/3/.hash
@@ -1 +1 @@
-54f935920ab0934c242145cf00f9852ae3f5a63e -
+54f935920ab0934c242145cf00f9852ae3f5a63e
diff --git a/build/aidl_api/test-piece-2/1/.hash b/build/aidl_api/test-piece-2/1/.hash
index b1723ae..705fc31 100644
--- a/build/aidl_api/test-piece-2/1/.hash
+++ b/build/aidl_api/test-piece-2/1/.hash
@@ -1 +1 @@
-cf529419c2c68a8d00fa58b29de46ebdbcabcc10 -
+fcd36db451cdbeeb049833fd7f499a987acf3930
diff --git a/build/aidl_api/test-piece-3/1/.hash b/build/aidl_api/test-piece-3/1/.hash
index d14b650..f85682c 100644
--- a/build/aidl_api/test-piece-3/1/.hash
+++ b/build/aidl_api/test-piece-3/1/.hash
@@ -1 +1 @@
-90488eb4c5ea0ae9de6f414383fdf007780b58b9 -
+f630cb120fa36f9f2507d730513a03b0cd4fbc0e
diff --git a/build/aidl_api/test-piece-4/1/.hash b/build/aidl_api/test-piece-4/1/.hash
index f9e9ffc..2982016 100644
--- a/build/aidl_api/test-piece-4/1/.hash
+++ b/build/aidl_api/test-piece-4/1/.hash
@@ -1 +1 @@
-b23b9616db460f76a75d265c329e097c508ca741 -
+29d4015ee2a4171396aecd925156aa2e47a42013
diff --git a/build/aidl_api/test-root-package/1/.hash b/build/aidl_api/test-root-package/1/.hash
index 5795162..8cf35a7 100644
--- a/build/aidl_api/test-root-package/1/.hash
+++ b/build/aidl_api/test-root-package/1/.hash
@@ -1 +1 @@
-3a4db987a854936c551a3fb3f889f8bf561ab9ab -
+1f59d8099e05dcc6ac47a9a12a1f2660dd60cfef
diff --git a/build/aidl_api/test-root-package/2/.hash b/build/aidl_api/test-root-package/2/.hash
index 9f6e7dc..5f9d7d9 100644
--- a/build/aidl_api/test-root-package/2/.hash
+++ b/build/aidl_api/test-root-package/2/.hash
@@ -1 +1 @@
-7025642541e73177f105c6d5b62a980fa5b4ace3 -
+7025642541e73177f105c6d5b62a980fa5b4ace3
diff --git a/build/aidl_interface.go b/build/aidl_interface.go
index f47cac5..cf7055e 100644
--- a/build/aidl_interface.go
+++ b/build/aidl_interface.go
@@ -77,7 +77,7 @@
aidlDumpApiRule = pctx.StaticRule("aidlDumpApiRule", blueprint.RuleParams{
Command: `rm -rf "${outDir}" && mkdir -p "${outDir}" && ` +
`${aidlCmd} --dumpapi --structured ${imports} ${optionalFlags} --out ${outDir} ${in} && ` +
- `(cd ${outDir} && find ./ -name "*.aidl" -print0 | LC_ALL=C sort -z | xargs -0 sha1sum && echo ${latestVersion}) | sha1sum > ${hashFile} `,
+ `(cd ${outDir} && find ./ -name "*.aidl" -print0 | LC_ALL=C sort -z | xargs -0 sha1sum && echo ${latestVersion}) | sha1sum | cut -d " " -f 1 > ${hashFile} `,
CommandDeps: []string{"${aidlCmd}"},
}, "optionalFlags", "imports", "outDir", "hashFile", "latestVersion")
@@ -108,10 +108,16 @@
}, "optionalFlags", "old", "new", "messageFile")
aidlDiffApiRule = pctx.StaticRule("aidlDiffApiRule", blueprint.RuleParams{
- Command: `(diff -N --line-format="" ${oldHashFile} ${newHashFile} && diff -r -B -I '//.*' ${old} ${new} && touch ${out}) || ` +
- `(cat ${messageFile} && exit 1)`,
+ Command: `if diff -r -B -I '//.*' '${old}'/* '${new}'/*; then touch '${out}'; else ` +
+ `cat '${messageFile}' && exit 1; fi`,
Description: "Check equality of ${new} and ${old}",
- }, "old", "new", "messageFile", "oldHashFile", "newHashFile")
+ }, "old", "new", "messageFile")
+
+ aidlVerifyHashRule = pctx.StaticRule("aidlVerifyHashRule", blueprint.RuleParams{
+ Command: `if [ $$(cd '${apiDir}' && { find ./ -name "*.aidl" -print0 | LC_ALL=C sort -z | xargs -0 sha1sum && echo ${version}; } | sha1sum | cut -d " " -f 1) = $$(read -r <'${hashFile}' hash extra; printf %s $$hash) ]; then ` +
+ `touch ${out}; else cat '${messageFile}' && exit 1; fi`,
+ Description: "Verify ${apiDir} files have not been modified",
+ }, "apiDir", "version", "messageFile", "hashFile")
joinJsonObjectsToArrayRule = pctx.StaticRule("joinJsonObjectsToArrayRule", blueprint.RuleParams{
Rspfile: "$out.rsp",
@@ -237,6 +243,9 @@
for _, path := range api.checkApiTimestamps {
g.implicitInputs = append(g.implicitInputs, path)
}
+ for _, path := range api.checkHashTimestamps {
+ g.implicitInputs = append(g.implicitInputs, path)
+ }
}
})
g.importFlags = strings.Join(wrap("-I", importPaths, ""), " ")
@@ -424,6 +433,9 @@
// for triggering updating current API
updateApiTimestamp android.WritablePath
+ // for triggering check that files have not been modified
+ checkHashTimestamps android.WritablePaths
+
// for triggering freezing API as the new version
freezeApiTimestamp android.WritablePath
}
@@ -512,24 +524,23 @@
timestampFile := android.PathForModuleOut(ctx, "updateapi_"+version+".timestamp")
modulePath := android.PathForModuleSrc(ctx).String()
- apiPreamble := android.PathForSource(ctx, "system/tools/aidl/build/api_preamble.txt")
targetDir := filepath.Join(modulePath, m.apiDir(), version)
rb := android.NewRuleBuilder()
// Wipe the target directory and then copy the API dump into the directory
rb.Command().Text("mkdir -p " + targetDir)
rb.Command().Text("rm -rf " + targetDir + "/*")
- rb.Command().Text("cp -rf " + dump.dir.String() + "/. " + targetDir).Implicits(dump.files)
if version != currentVersion {
+ rb.Command().Text("cp -rf " + dump.dir.String() + "/. " + targetDir).Implicits(dump.files)
// If this is making a new frozen (i.e. non-current) version of the interface,
- // Place a preamble to prevent people from accidentally modifying the dumps.
- // And also modify Android.bp file to add the new version to the 'versions' property.
- rb.Command().Text("find " + targetDir + " -type f -name \"*.aidl\"").
- Text("| xargs -n 1 bash -c 'cat ").Input(apiPreamble).Text(" $0 > $0.temp && mv $0.temp $0'")
+ // modify Android.bp file to add the new version to the 'versions' property.
rb.Command().BuiltTool(ctx, "bpmodify").
Text("-w -m " + m.properties.BaseName).
Text("-parameter versions -a " + version).
Text(android.PathForModuleSrc(ctx, "Android.bp").String())
+ } else {
+ // In this case (unfrozen interface), don't copy .hash
+ rb.Command().Text("cp -rf " + dump.dir.String() + "/* " + targetDir).Implicits(dump.files)
}
rb.Command().Text("touch").Output(timestampFile)
@@ -588,10 +599,6 @@
implicits = append(implicits, oldDump.files...)
implicits = append(implicits, newDump.files...)
implicits = append(implicits, formattedMessageFile)
- if oldDump.hashFile.Valid() {
- implicits = append(implicits, oldDump.hashFile.Path())
- }
- implicits = append(implicits, newDump.hashFile.Path())
ctx.ModuleBuild(pctx, android.ModuleBuildParams{
Rule: aidlDiffApiRule,
Implicits: implicits,
@@ -600,8 +607,36 @@
"old": oldDump.dir.String(),
"new": newDump.dir.String(),
"messageFile": formattedMessageFile.String(),
- "oldHashFile": oldDump.hashFile.String(),
- "newHashFile": newDump.hashFile.String(),
+ },
+ })
+ return timestampFile
+}
+
+func (m *aidlApi) checkIntegrity(ctx android.ModuleContext, dump apiDump) android.WritablePath {
+ version := dump.dir.Base()
+ timestampFile := android.PathForModuleOut(ctx, "checkhash_"+version+".timestamp")
+ messageFile := android.PathForSource(ctx, "system/tools/aidl/build/message_check_integrity.txt")
+
+ i, _ := strconv.Atoi(version)
+ if i == 1 {
+ version = "latest-version"
+ } else {
+ version = strconv.Itoa(i - 1)
+ }
+
+ var implicits android.Paths
+ implicits = append(implicits, dump.files...)
+ implicits = append(implicits, dump.hashFile.Path())
+ implicits = append(implicits, messageFile)
+ ctx.ModuleBuild(pctx, android.ModuleBuildParams{
+ Rule: aidlVerifyHashRule,
+ Implicits: implicits,
+ Output: timestampFile,
+ Args: map[string]string{
+ "apiDir": dump.dir.String(),
+ "version": version,
+ "hashFile": dump.hashFile.Path().String(),
+ "messageFile": messageFile.String(),
},
})
return timestampFile
@@ -657,6 +692,11 @@
dumps = append(dumps, currentApiDump)
}
for i, _ := range dumps {
+ if dumps[i].hashFile.Valid() {
+ checkHashTimestamp := m.checkIntegrity(ctx, dumps[i])
+ m.checkHashTimestamps = append(m.checkHashTimestamps, checkHashTimestamp)
+ }
+
if i == 0 {
continue
}
@@ -1297,9 +1337,12 @@
// avoid needing to filter out duplicates.
info.HashFiles = android.FirstUniqueStrings(info.HashFiles)
+ implicits := android.PathsForSource(ctx, info.HashFiles)
+
ctx.Build(pctx, android.BuildParams{
- Rule: aidlMetadataRule,
- Output: metadataPath,
+ Rule: aidlMetadataRule,
+ Implicits: implicits,
+ Output: metadataPath,
Args: map[string]string{
"name": name,
"stability": info.Stability,
diff --git a/build/aidl_test.go b/build/aidl_test.go
index 547df47..62761f8 100644
--- a/build/aidl_test.go
+++ b/build/aidl_test.go
@@ -96,13 +96,13 @@
fs := map[string][]byte{
"a.java": nil,
"AndroidManifest.xml": nil,
- "build/make/target/product/security/testkey": nil,
- "framework/aidl/a.aidl": nil,
- "IFoo.aidl": nil,
- "libbinder_ndk.map.txt": nil,
- "system/tools/aidl/build/api_preamble.txt": nil,
+ "build/make/target/product/security/testkey": nil,
+ "framework/aidl/a.aidl": nil,
+ "IFoo.aidl": nil,
+ "libbinder_ndk.map.txt": nil,
"system/tools/aidl/build/message_check_compatibility.txt": nil,
"system/tools/aidl/build/message_check_equality.txt": nil,
+ "system/tools/aidl/build/message_check_integrity.txt": nil,
}
cc.GatherRequiredFilesForTest(fs)
@@ -230,6 +230,7 @@
}
`, withFiles(map[string][]byte{
"aidl_api/foo/1/foo.1.aidl": nil,
+ "aidl_api/foo/1/.hash": nil,
}))
// For alias for the latest frozen version (=1)
@@ -291,7 +292,9 @@
}
`, withFiles(map[string][]byte{
"aidl_api/foo/1/foo.1.aidl": nil,
+ "aidl_api/foo/1/.hash": nil,
"aidl_api/foo/2/foo.2.aidl": nil,
+ "aidl_api/foo/2/.hash": nil,
}))
// alias for the latest frozen version (=2)
diff --git a/build/api_preamble.txt b/build/api_preamble.txt
deleted file mode 100644
index 3a2c34d..0000000
--- a/build/api_preamble.txt
+++ /dev/null
@@ -1,17 +0,0 @@
-///////////////////////////////////////////////////////////////////////////////
-// THIS FILE IS IMMUTABLE. DO NOT EDIT IN ANY CASE. //
-///////////////////////////////////////////////////////////////////////////////
-
-// This file is a frozen snapshot of an AIDL interface (or parcelable). Do not
-// try to edit this file. It looks like you are doing that because you have
-// modified an AIDL interface in a backward-incompatible way, e.g., deleting a
-// function from an interface or a field from a parcelable and it broke the
-// build. That breakage is intended.
-//
-// You must not make a backward incompatible changes to the AIDL files built
-// with the aidl_interface module type with versions property set. The module
-// type is used to build AIDL files in a way that they can be used across
-// independently updatable components of the system. If a device is shipped
-// with such a backward incompatible change, it has a high risk of breaking
-// later when a module using the interface is updated, e.g., Mainline modules.
-
diff --git a/build/message_check_integrity.txt b/build/message_check_integrity.txt
new file mode 100644
index 0000000..57aaa90
--- /dev/null
+++ b/build/message_check_integrity.txt
@@ -0,0 +1,7 @@
+###############################################################################
+# ERROR: Modification detected of stable AIDL API file #
+###############################################################################
+Above AIDL file(s) has changed, resulting in a different hash. Hash values may
+be checked at runtime to verify interface stability. If a device is shipped
+with this change by ignoring this message, it has a high risk of breaking later
+when a module using the interface is updated, e.g., Mainline modules.
diff --git a/metadata/test.cpp b/metadata/test.cpp
index 9daf1b1..67d08b2 100644
--- a/metadata/test.cpp
+++ b/metadata/test.cpp
@@ -37,7 +37,7 @@
EXPECT_THAT(info->types,
ElementsAre("some_package.IFoo", "some_package.Thing",
"some_package.sub_package.IFoo", "some_package.sub_package.SubThing"));
- EXPECT_THAT(info->hashes, ElementsAre("db5ac8f6ff15c327e0da41396ba7af9ec21fc48c",
+ EXPECT_THAT(info->hashes, ElementsAre("13e24b2fac6a979971819fba2ab0d6d7c4182122",
"dc2a9292847e43b4360bb183f7491f0e9895eaa9",
"54f935920ab0934c242145cf00f9852ae3f5a63e"));
}
