From a866829885570e2a925637bf53f36eb8ac689eef Mon Sep 17 00:00:00 2001 From: Dan Albert Date: Mon, 5 Aug 2024 23:58:48 +0000 Subject: [PATCH] Verify that ndk_headers headers are valid C. https://github.com/android/ndk/issues/1920 recently reported a handful of NDK headers where C-incompatible syntax had slipped in which wasn't caught by review. This is also something that we often manually catch in API review, but the manual inspection is, as can be seen from that bug, error prone. This check is pretty stupid. I've tried on other occasions to do a more thorough check that would build each header using the same flags as cc_library or similar would, but was never able to get that working. This isn't as thorough, but that's maybe not such a bad thing, since it saves on build time to only check on variant. In practice the rudimentary approach I've taken here would have caught every instance reported in the bug above, and probably all the other cases that we'd previously caught in API review as well, so it's definitely a step in the right direction if not good enough forever. Bug: http://b/113359184 Test: m ndk Change-Id: Id4e8cc511413acc61c4f625f25c3004d7439263c --- cc/ndk_headers.go | 17 ++++++++- cc/ndk_sysroot.go | 89 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 104 insertions(+), 2 deletions(-) diff --git a/cc/ndk_headers.go b/cc/ndk_headers.go index 57a3b3a9c..e00293143 100644 --- a/cc/ndk_headers.go +++ b/cc/ndk_headers.go @@ -18,6 +18,7 @@ import ( "path/filepath" "android/soong/android" + "github.com/google/blueprint" ) @@ -44,7 +45,7 @@ func init() { } // Returns the NDK base include path for use with sdk_version current. Usable with -I. -func getCurrentIncludePath(ctx android.ModuleContext) android.OutputPath { +func getCurrentIncludePath(ctx android.PathContext) android.OutputPath { return getNdkSysrootBase(ctx).Join(ctx, "usr/include") } @@ -73,6 +74,13 @@ type headerProperties struct { // Path to the NOTICE file associated with the headers. License *string `android:"path"` + + // Set to true if the headers installed by this module should skip + // verification. This step ensures that each header is self-contained (can + // be #included alone) and is valid C. This should not be disabled except in + // rare cases. Outside bionic and external, if you're using this option + // you've probably made a mistake. + Skip_verification *bool } type headerModule struct { @@ -309,6 +317,13 @@ type preprocessedHeadersProperties struct { // Path to the NOTICE file associated with the headers. License *string + + // Set to true if the headers installed by this module should skip + // verification. This step ensures that each header is self-contained (can + // be #included alone) and is valid C. This should not be disabled except in + // rare cases. Outside bionic and external, if you're using this option + // you've probably made a mistake. + Skip_verification *bool } type preprocessedHeadersModule struct { diff --git a/cc/ndk_sysroot.go b/cc/ndk_sysroot.go index 3c48f6881..f571523a0 100644 --- a/cc/ndk_sysroot.go +++ b/cc/ndk_sysroot.go @@ -54,7 +54,22 @@ package cc import ( "android/soong/android" + "fmt" + "path/filepath" "strings" + + "github.com/google/blueprint" +) + +var ( + verifyCCompat = pctx.AndroidStaticRule("verifyCCompat", + blueprint.RuleParams{ + Command: "$ccCmd -x c -fsyntax-only $flags $in && touch $out", + CommandDeps: []string{"$ccCmd"}, + }, + "ccCmd", + "flags", + ) ) func init() { @@ -103,6 +118,45 @@ func getNdkABIHeadersFile(ctx android.PathContext) android.WritablePath { return android.PathForOutput(ctx, "ndk_abi_headers.txt") } +func verifyNdkHeaderIsCCompatible(ctx android.SingletonContext, + src android.Path, dest android.Path) android.Path { + sysrootInclude := getCurrentIncludePath(ctx) + baseOutputDir := android.PathForOutput(ctx, "c-compat-verification") + installRelPath, err := filepath.Rel(sysrootInclude.String(), dest.String()) + if err != nil { + ctx.Errorf("filepath.Rel(%q, %q) failed: %s", dest, sysrootInclude, err) + } + output := baseOutputDir.Join(ctx, installRelPath) + ctx.Build(pctx, android.BuildParams{ + Rule: verifyCCompat, + Description: fmt.Sprintf("Verifying C compatibility of %s", src), + Output: output, + Input: dest, + // Ensures that all the headers in the sysroot are already installed + // before testing any of the headers for C compatibility, and also that + // the check will be re-run whenever the sysroot changes. This is + // necessary because many of the NDK headers depend on other NDK + // headers, but we don't have explicit dependency tracking for that. + Implicits: []android.Path{getNdkHeadersTimestampFile(ctx)}, + Args: map[string]string{ + "ccCmd": "${config.ClangBin}/clang", + "flags": fmt.Sprintf( + // Ideally we'd check each ABI, multiple API levels, + // fortify/non-fortify, and a handful of other variations. It's + // a lot more difficult to do that though, and would eat up more + // build time. All the problems we've seen so far that this + // check would catch have been in arch-generic and + // minSdkVersion-generic code in frameworks though, so this is a + // good place to start. + "-target aarch64-linux-android%d --sysroot %s", + android.FutureApiLevel.FinalOrFutureInt(), + getNdkSysrootBase(ctx).String(), + ), + }, + }) + return output +} + func NdkSingleton() android.Singleton { return &ndkSingleton{} } @@ -143,10 +197,17 @@ func writeNdkAbiSrcFilter(ctx android.BuilderContext, type ndkSingleton struct{} +type srcDestPair struct { + src android.Path + dest android.Path +} + func (n *ndkSingleton) GenerateBuildActions(ctx android.SingletonContext) { var staticLibInstallPaths android.Paths var headerSrcPaths android.Paths var headerInstallPaths android.Paths + var headersToVerify []srcDestPair + var headerCCompatVerificationTimestampPaths android.Paths var installPaths android.Paths var licensePaths android.Paths ctx.VisitAllModules(func(module android.Module) { @@ -157,6 +218,14 @@ func (n *ndkSingleton) GenerateBuildActions(ctx android.SingletonContext) { if m, ok := module.(*headerModule); ok { headerSrcPaths = append(headerSrcPaths, m.srcPaths...) headerInstallPaths = append(headerInstallPaths, m.installPaths...) + if !Bool(m.properties.Skip_verification) { + for i, installPath := range m.installPaths { + headersToVerify = append(headersToVerify, srcDestPair{ + src: m.srcPaths[i], + dest: installPath, + }) + } + } installPaths = append(installPaths, m.installPaths...) licensePaths = append(licensePaths, m.licensePath) } @@ -164,6 +233,10 @@ func (n *ndkSingleton) GenerateBuildActions(ctx android.SingletonContext) { if m, ok := module.(*versionedHeaderModule); ok { headerSrcPaths = append(headerSrcPaths, m.srcPaths...) headerInstallPaths = append(headerInstallPaths, m.installPaths...) + // Verification intentionally not done for headers that go through + // versioner. It'd be nice to have, but the only user is bionic, and + // that one module would also need to use skip_verification, so it + // wouldn't help at all. installPaths = append(installPaths, m.installPaths...) licensePaths = append(licensePaths, m.licensePath) } @@ -171,6 +244,14 @@ func (n *ndkSingleton) GenerateBuildActions(ctx android.SingletonContext) { if m, ok := module.(*preprocessedHeadersModule); ok { headerSrcPaths = append(headerSrcPaths, m.srcPaths...) headerInstallPaths = append(headerInstallPaths, m.installPaths...) + if !Bool(m.properties.Skip_verification) { + for i, installPath := range m.installPaths { + headersToVerify = append(headersToVerify, srcDestPair{ + src: m.srcPaths[i], + dest: installPath, + }) + } + } installPaths = append(installPaths, m.installPaths...) licensePaths = append(licensePaths, m.licensePath) } @@ -223,6 +304,12 @@ func (n *ndkSingleton) GenerateBuildActions(ctx android.SingletonContext) { Implicits: headerInstallPaths, }) + for _, srcDestPair := range headersToVerify { + headerCCompatVerificationTimestampPaths = append( + headerCCompatVerificationTimestampPaths, + verifyNdkHeaderIsCCompatible(ctx, srcDestPair.src, srcDestPair.dest)) + } + writeNdkAbiSrcFilter(ctx, headerSrcPaths, getNdkABIHeadersFile(ctx)) fullDepPaths := append(staticLibInstallPaths, getNdkBaseTimestampFile(ctx)) @@ -235,6 +322,6 @@ func (n *ndkSingleton) GenerateBuildActions(ctx android.SingletonContext) { ctx.Build(pctx, android.BuildParams{ Rule: android.Touch, Output: getNdkFullTimestampFile(ctx), - Implicits: fullDepPaths, + Implicits: append(fullDepPaths, headerCCompatVerificationTimestampPaths...), }) }