Closed Bug 1294731 Opened 3 years ago Closed 3 years ago

Validate cached libraries based on checksums

Categories

(Firefox for Android :: General, defect)

51 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: esawin, Assigned: esawin)

References

Details

Attachments

(2 files, 12 obsolete files)

2.00 KB, patch
esawin
: review+
Details | Diff | Splinter Review
5.71 KB, patch
esawin
: review+
Details | Diff | Splinter Review
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.
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)
Attachment #8781155 - Flags: review?(mh+mozilla)
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)
Attachment #8781160 - Flags: review?(mh+mozilla)
Compute and package CRC codes alongside the shared libraries into <assets-path>/<cpu-arch>/<lib-name>.crc.
Attachment #8781163 - Flags: review?(mh+mozilla)
Prevent MappableExtractFile from auto-unlinking on destruction. We want to make the cached files persistent.
Attachment #8781164 - Flags: review?(mh+mozilla)
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 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.
Attachment #8781160 - Flags: review?(mh+mozilla)
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).
Attachment #8781155 - Flags: review?(mh+mozilla)
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.
Attachment #8781163 - Flags: review?(mh+mozilla)
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.
Attachment #8781164 - Flags: review?(mh+mozilla)
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)
Attachment #8781160 - Attachment is obsolete: true
Attachment #8781163 - Attachment is obsolete: true
Attachment #8781984 - Flags: review?(mh+mozilla)
Attachment #8781984 - Flags: feedback?(snorp)
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
Attachment #8781984 - Flags: feedback?(snorp) → feedback-
Addressed comments and reduced some code.
Attachment #8781155 - Attachment is obsolete: true
Attachment #8781164 - Attachment is obsolete: true
Attachment #8781984 - Attachment is obsolete: true
Attachment #8781984 - Flags: review?(mh+mozilla)
Attachment #8782036 - Flags: review?(snorp)
Attachment #8782036 - Flags: review?(mh+mozilla)
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
Attachment #8782036 - Flags: review?(snorp) → review+
Addressed comments.
Attachment #8782036 - Attachment is obsolete: true
Attachment #8782036 - Flags: review?(mh+mozilla)
Attachment #8782100 - Flags: review?(mh+mozilla)
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.
Attachment #8782105 - Flags: review?(mh+mozilla)
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.
Attachment #8782100 - Flags: review?(mh+mozilla)
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?
Attachment #8782105 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8782100 - Attachment is obsolete: true
Attachment #8782188 - Flags: review?(mh+mozilla)
The rest of the POSIX types are covered by pthread.h, so we can remove the sys/types.h include.
Attachment #8782105 - Attachment is obsolete: true
Attachment #8782189 - Flags: review+
s/nextDir/nextFile
Attachment #8782188 - Attachment is obsolete: true
Attachment #8782188 - Flags: review?(mh+mozilla)
Attachment #8782390 - Flags: review?(mh+mozilla)
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.
Attachment #8782390 - Flags: review?(mh+mozilla)
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.
Attachment #8782390 - Attachment is obsolete: true
Attachment #8783609 - Flags: review?(snorp)
Attachment #8783609 - Flags: review?(snorp) → review?(mh+mozilla)
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...
Attachment #8783609 - Flags: review?(mh+mozilla) → feedback+
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.
Attachment #8783609 - Attachment is obsolete: true
Attachment #8784051 - Flags: review?(mh+mozilla)
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
Attachment #8784051 - Flags: review?(mh+mozilla) → review+
Pushed by esawin@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/70b57c7fe9b0
https://hg.mozilla.org/mozilla-central/rev/f043648dd349
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.