1294731 - Validate cached libraries based on checksums

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Eugen Sawin [:esawin]]

Currently, cached libraries are validated based on system file timestamps (time of last modification comparison between the APK and the library), which can be unreliable.

We want an alternative method to validate cached libraries independent from system-derived properties.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1294731-1.1-Move-APK-assets-path-into-a-build-ti.patch

Consolidate the APK assets path definition in an environment variable to prevent redundant re-definitions (the validation code will add another reference to it in Mappable.cpp)

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1294731-2.1-Validate-cached-libraries-based-on-c.patch

The CacheValidator provides following validation mechanics:

1. Check for an existing <assets-path>/<cpu-arch>/<lib-name>.crc file and compares it against <cache-path>/<lib-name>.crc.
a) If no CRC file exists in the APK, move to step 2. 
b) If no cached CRC file exists or the CRC files differ, invalidate cache, move to step 3.
c) Otherwise, reuse cached libraries.
2. Use timestamp-based validation fallback.
3. Cache APK checksum in <cache-path>/<lib-name>.cc, if it exists.

(Plus some whitespace fixes)

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1294731-3.1-Compute-and-package-CRC-codes-alongs.patch

Compute and package CRC codes alongside the shared libraries into <assets-path>/<cpu-arch>/<lib-name>.crc.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1294731-4.1-Don-t-unlink-cached-files-on-shutdow.patch

Prevent MappableExtractFile from auto-unlinking on destruction. We want to make the cached files persistent.

Comment 5

[Eugen Sawin [:esawin]]

To clarify on comment 2, the "APK checksum" is the CRC code for the given library provided in the APK, it's not a CRC code of the APK itself.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8781160 [details] [diff] [review]
0002-Bug-1294731-2.1-Validate-cached-libraries-based-on-c.patch

Review of attachment 8781160 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/Mappable.cpp
@@ +31,5 @@
> +{
> +  static const size_t kMaxChecksumLen = 8;
> +  static constexpr char* kChecksumSuffix = ".crc";
> +  static constexpr char* kApkAssetsPath =
> +    ANDROID_ASSETS_PATH "/" ANDROID_CPU_ARCH "/";

I don't see why you want to hardcode the assets path, when that path is not hardcoded when reading the corresponding library. So the file would just have the same base path as the library being loaded.

But in fact, you don't need a crc stored in the apk at all. The APK *already* has a crc for the file. That's why snorp suggested using the crc in the first place: because it's already there.

@@ +198,4 @@
>      ERROR("Couldn't open %s to decompress library", path.get());
>      return nullptr;
>    }
> +  AutoUnlinkFile file(path.get());

When path goes out of scope, it's doing to be delete()d. And the reference kept in the AutoUnlinkFile will be invalid. Plus, the AutoUnlinkFile is going to delete() it as well.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8781155 [details] [diff] [review]
0001-Bug-1294731-1.1-Move-APK-assets-path-into-a-build-ti.patch

Review of attachment 8781155 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this is needed (cf. other review).

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8781163 [details] [diff] [review]
0003-Bug-1294731-3.1-Compute-and-package-CRC-codes-alongs.patch

Review of attachment 8781163 [details] [diff] [review]:
-----------------------------------------------------------------

As mentioned in the other review: there already is a CRC of each file in the APK.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8781164 [details] [diff] [review]
0004-Bug-1294731-4.1-Don-t-unlink-cached-files-on-shutdow.patch

Review of attachment 8781164 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/Mappable.cpp
@@ -198,4 @@
>      ERROR("Couldn't open %s to decompress library", path.get());
>      return nullptr;
>    }
> -  AutoUnlinkFile file(path.get());

Auto-unlink when something wrong happens during decompression is still desirable.

Comment 10

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1294731-2.2-Validate-cached-libraries-based-on-c.patch

Shipping extra CRC files alongside the libraries is the approach I've discussed with snorp. I'm sorry if we have communicated this badly.

This patch implements your (glandium) proposal of reusing the SHA1 digests (assuming that's what you referred to as CRC) as generated by jarsigner during packaging.

The advantages of this approach are:
+ No checksum computation during packaging
+ No additional files shipped (8 bytes + file overhead per library)
+ Only one file access during checksum validation

The disadvantages are:
- More fragile (format-dependent parser) and complex code
- Dependent on jarsigner output format
- SHA1 digests are based on compressed libraries, no way to validate cached libraries on device in future
- The manifest file is compressed, needs decompression and parsing (140kb) at startup and cannot be cached (unless we revert to timestamp-based validation)

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8781984 [details] [diff] [review]
0002-Bug-1294731-2.2-Validate-cached-libraries-based-on-c.patch

Review of attachment 8781984 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly good but I think we need a few tweaks

::: mozglue/linker/Mappable.cpp
@@ +37,5 @@
> +public:
> +  // Ensures that the manifest has been parsed and the checksum map initialized.
> +  void EnsureInit(const Zip& aZip)
> +  {
> +    if (mIsInitializing || !mMap.empty()) {

We guard against reentrancy here but I don't think we need to, see below...

@@ +72,5 @@
> +
> +    RefPtr<Mappable> mappable;
> +    Zip::Stream stream;
> +    if (aZip.GetStream(manifestPath.get(), &stream)) {
> +      mappable = MappableExtractFile::Create(kManifestName, "", &aZip, &stream,

If we just use MappableDeflate here instead, we avoid reentrancy and can simplify some stuff

@@ +120,5 @@
> +    }
> +  }
> +
> +  std::unordered_map<std::string, std::string> mMap;
> +  bool mIsInitializing;

Can remove this per above

@@ +126,5 @@
> +
> +class CacheValidator
> +{
> +public:
> +  CacheValidator(const char* aZipPath, const char* aCachedPath, const Zip& aZip)

This is different from mCachedPath that we create below and the naming confused me a bit. Maybe use aCachedLibPath, aZipLibPath to be more clear? Also mCachedSumPath? Not sure.

@@ +149,5 @@
> +    // 1. checksums
> +    // 2. system timestamps
> +
> +    // Validated based on checksum.
> +    RefPtr<Mappable> crcMappable = MappableFile::Create(mCachedPath.get());

Not a crc

@@ +169,5 @@
> +      }
> +      return !memcmp(crcBuf, mChecksum.c_str(), mChecksum.length());
> +    }
> +
> +    // No checksum found, validate based on timestamp.

I think this should just abort, actually. If the manifest isn't found or the entry isn't found it it means something went very wrong.

@@ +198,5 @@
> +    }
> +
> +    DEBUG_LOG("updating checksum %s", mCachedPath.get());
> +
> +    MappedPtr crcBuf(MemoryRange::mmap(nullptr, mChecksum.length(), PROT_WRITE,

Not a crc

::: mozglue/linker/Mappable.h
@@ +117,5 @@
>     * Create a MappableExtractFile instance for the given Zip stream. The name
>     * argument is used to create the cache file in the cache directory.
>     */
> +  static Mappable* Create(const char* name, const char* subpath, const Zip* zip,
> +                          Zip::Stream* stream, bool allowCached);

Don't need allowCached if you are guarding against reentrancy anyway

Comment 12

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1294731-2.3-Validate-cached-libraries-based-on-c.patch

Addressed comments and reduced some code.

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8782036 [details] [diff] [review]
0002-Bug-1294731-2.3-Validate-cached-libraries-based-on-c.patch

Review of attachment 8782036 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me with a couple nits

::: mozglue/linker/Mappable.cpp
@@ +254,5 @@
>    sprintf(path.get(), "%s/%s", cachePath, name);
> +
> +  CacheValidator validator(subpath, path.get(), zip);
> +  if (validator.IsValid()) {
> +    ERROR("Reusing %s", static_cast<char *>(path.get()));

s/ERROR/DEBUG_LOG/

::: mozglue/linker/Mappable.h
@@ +103,4 @@
>    AutoCloseFD fd;
>  };
>  
> +

Just nuke both of these whitespace changes

Comment 14

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1294731-2.4-Validate-cached-libraries-based-on-c.patch

Addressed comments.

Comment 15

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1294731-4.2-Don-t-unlink-cached-extracted-files-.patch

I think this is what we want: keep AutoUnlinkFile for the intermediate processing in MEF::Create and then move ownership to MEF via its constructor.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8782100 [details] [diff] [review]
0001-Bug-1294731-2.4-Validate-cached-libraries-based-on-c.patch

Review of attachment 8782100 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/Mappable.cpp
@@ +29,4 @@
>  using mozilla::MakeUnique;
>  using mozilla::UniquePtr;
>  
> +// Used to parse META-INF/MANIFEST.MF from the APK and provide access to the

I should have made myself clearer, sorry. I meant you should use the existing *CRC32* from the APK itself, not the sha1s stored in a MANIFEST.MF in the APK.

Add a CRC32 field to Zip::Stream, fill the field from the existing CRC32 field in what nextFile/nextDir in Zip::GetStream return, and use that.

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8782105 [details] [diff] [review]
0002-Bug-1294731-4.2-Don-t-unlink-cached-extracted-files-.patch

Review of attachment 8782105 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/Mappable.h
@@ +137,5 @@
>    };
>    typedef mozilla::UniquePtr<char[], UnlinkFile> AutoUnlinkFile;
>  
> +  MappableExtractFile(int fd, const char* path)
> +  : MappableFile(fd), path(path) { }

Removing the use of pid_t might be making the sys/types.h include unnecessary. Can you try removing it and see if that still compiles?

Comment 18

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1294731-2.5-Validate-cached-libraries-based-on-c.patch

This makes so much more sense than having me write a manifest parser to validate checksums! I didn't know about the provided CRC codes in the zip.

This should do it.

Comment 19

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1294731-4.3-Don-t-unlink-cached-extracted-files-.patch

The rest of the POSIX types are covered by pthread.h, so we can remove the sys/types.h include.

Comment 20

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1294731-2.5-Validate-cached-libraries-based-on-c.patch

s/nextDir/nextFile

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 8782390 [details] [diff] [review]
0001-Bug-1294731-2.5-Validate-cached-libraries-based-on-c.patch

Review of attachment 8782390 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/Mappable.cpp
@@ +33,5 @@
> +
> +public:
> +  CacheValidator(const char* aCachedLibPath, Zip* aZip, Zip::Stream* aStream)
> +  {
> +    static const char* kChecksumSuffix = ".crc";

static const char kChecksumSuffix[] = ".crc";

@@ +36,5 @@
> +  {
> +    static const char* kChecksumSuffix = ".crc";
> +
> +    mCachedChecksumPath =
> +      MakeUnique<char[]>(strlen(aCachedLibPath) + strlen(kChecksumSuffix) + 1);

sizeof(kChecksumSuffix), and then you don't need +1.

@@ +41,5 @@
> +    sprintf(mCachedChecksumPath.get(), "%s%s", aCachedLibPath, kChecksumSuffix);
> +    DEBUG_LOG("mCachedChecksumPath: %s", mCachedChecksumPath.get());
> +
> +    mChecksum = MakeUnique<char[]>(kChecksumLen + 1);
> +    sprintf(mChecksum.get(), "%8x", aStream->GetCRC32());

This would be simpler if you just dumped the raw in-memory value as 4 raw bytes in the file. I don't think it's worth bothering making it human readable when one can just use hexdump if they really want to look at the crc value.

@@ +59,5 @@
> +    DEBUG_LOG("comparing %s with content in %s", mChecksum.get(), mCachedChecksumPath.get());
> +    MappedPtr checksumBuf = checksumMappable->mmap(nullptr, checksumMappable->GetLength(),
> +                                                   PROT_READ, MAP_PRIVATE, 0);
> +    if (checksumBuf == MAP_FAILED) {
> +      ERROR("Couldn't map %s to validate checksum", mCachedChecksumPath.get());

WARN here and below. ERROR is really meant for events that prevent things from going forward. Here, the library is still going to be decompressed.

@@ +72,5 @@
> +    AutoCloseFD fd(open(mCachedChecksumPath.get(),
> +                        O_TRUNC | O_RDWR | O_CREAT | O_NOATIME,
> +                        S_IRUSR | S_IWUSR));
> +
> +    if (fd == -1  || ftruncate(fd, kChecksumLen) == -1) {

You're opening with O_TRUNC, which is going to empty the file. Then you write kChecksumLen bytes to it, so it will be that size. You don't need to ftruncate(). Yes, even if write() fails, the mmap will not fail because the file is empty, and the address it returns will be readable. You'd just get nul characters. (you can double check by removing the write and see what happens)

@@ +77,5 @@
> +      ERROR("Couldn't ftruncate %s to update checksum", mCachedChecksumPath.get());
> +      return;
> +    }
> +    DEBUG_LOG("updating checksum %s", mCachedChecksumPath.get());
> +    write(fd, mChecksum.get(), kChecksumLen);

POSIX is painful. You'll want to loop while write returns -1 and errno is EINTR.

Comment 22

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1294731-2.6-Validate-cached-libraries-based-on-c.patch

Addressed comments.
I've switched the POSIX write call to mmap/memcpy to keep it more consistent with the rests of the linker code and error reporting.

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 8783609 [details] [diff] [review]
0001-Bug-1294731-2.6-Validate-cached-libraries-based-on-c.patch

Review of attachment 8783609 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozglue/linker/Mappable.cpp
@@ +76,5 @@
> +      return;
> +    }
> +    DEBUG_LOG("updating checksum %s", mCachedChecksumPath.get());
> +    MappedPtr buffer(MemoryRange::mmap(nullptr, size, PROT_WRITE, MAP_SHARED, fd, 0));
> +    memcpy(buffer, &mChecksum, size);

Even with the loop to handle EINTR, open+write would still be simpler and more desirable than open, ftruncate, mmap, memcpy...

Comment 24

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1294731-2.7-Validate-cached-libraries-based-on-c.patch

Use POSIX write instead.

EINTR is never set when writing files on Linux afaik, so not accepting 0 return value may be wrong in this case.

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8784051 [details] [diff] [review]
0001-Bug-1294731-2.7-Validate-cached-libraries-based-on-c.patch

Review of attachment 8784051 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the change below.

::: mozglue/linker/Mappable.cpp
@@ +80,5 @@
> +    while (written < size) {
> +      ssize_t ret = write(fd,
> +                          reinterpret_cast<const uint8_t*>(&mChecksum) + written,
> +                          size - written);
> +      if (ret > 0) {

>= 0

Comment 26

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1294731-2.8-Validate-cached-libraries-based-on-c.patch

Addressed comment.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70b57c7fe9b0
[2.8] Validate cached libraries based on checksums. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/f043648dd349
[4.3] Don't unlink cached extracted files on shutdown. r=glandium

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/70b57c7fe9b0
https://hg.mozilla.org/mozilla-central/rev/f043648dd349