From 9cdc71dfe38372c0bd7268d0b5f96cfb59d7c929 Mon Sep 17 00:00:00 2001 From: Ben Langmuir Date: Thu, 11 Dec 2025 09:38:22 -0800 Subject: [PATCH] [llvm][cas] Improve UnifiedOnDiskActionCache validation to check cas refs (#171732) Check that action cache references point to valid CAS objects by ensuring they are contained within the corresponding CAS and also that the offsets match. This prevents accidentally referencing "dead" index records that were not properly flushed to disk, which can lead to the action cache pointing to the wrong data or to garbage data. rdar://126642956 (cherry picked from commit a451ff04d20021f6a15d22aef585903c434baa9e) --- llvm/include/llvm/CAS/OnDiskGraphDB.h | 28 ++++++++++++------------ llvm/lib/CAS/OnDiskGraphDB.cpp | 25 ++++++++++++++++----- llvm/lib/CAS/UnifiedOnDiskCache.cpp | 17 ++++++++++++-- llvm/test/tools/llvm-cas/validation.test | 8 +++++++ 4 files changed, 57 insertions(+), 21 deletions(-) diff --git a/llvm/include/llvm/CAS/OnDiskGraphDB.h b/llvm/include/llvm/CAS/OnDiskGraphDB.h index 5b82a100ef062..bbe6482911162 100644 --- a/llvm/include/llvm/CAS/OnDiskGraphDB.h +++ b/llvm/include/llvm/CAS/OnDiskGraphDB.h @@ -278,7 +278,7 @@ class OnDiskGraphDB { /// /// Returns \p nullopt if the object is not stored in this CAS. LLVM_ABI_FOR_TEST std::optional - getExistingReference(ArrayRef Digest); + getExistingReference(ArrayRef Digest, bool CheckUpstream = true); /// Check whether the object associated with \p Ref is stored in the CAS. /// Note that this function will fault-in according to the policy. @@ -286,8 +286,15 @@ class OnDiskGraphDB { /// Check whether the object associated with \p Ref is stored in the CAS. /// Note that this function does not fault-in. - bool containsObject(ObjectID Ref) const { - return containsObject(Ref, /*CheckUpstream=*/true); + bool containsObject(ObjectID Ref, bool CheckUpstream = true) const { + switch (getObjectPresence(Ref, CheckUpstream)) { + case ObjectPresence::Missing: + return false; + case ObjectPresence::InPrimaryDB: + return true; + case ObjectPresence::OnlyInUpstreamDB: + return true; + } } /// \returns the data part of the provided object handle. @@ -360,17 +367,6 @@ class OnDiskGraphDB { LLVM_ABI_FOR_TEST ObjectPresence getObjectPresence(ObjectID Ref, bool CheckUpstream) const; - bool containsObject(ObjectID Ref, bool CheckUpstream) const { - switch (getObjectPresence(Ref, CheckUpstream)) { - case ObjectPresence::Missing: - return false; - case ObjectPresence::InPrimaryDB: - return true; - case ObjectPresence::OnlyInUpstreamDB: - return true; - } - } - /// When \p load is called for a node that doesn't exist, this function tries /// to load it from the upstream store and copy it to the primary one. Expected> faultInFromUpstream(ObjectID PrimaryID); @@ -404,6 +400,10 @@ class OnDiskGraphDB { IndexProxy getIndexProxyFromRef(InternalRef Ref) const; + // FIXME: on newer branches we have refactored getIndexProxyFromRef to return + // Expected. As a stop gap, provide a checked API. + Expected getIndexProxyFromRefChecked(InternalRef Ref) const; + static InternalRef makeInternalRef(FileOffset IndexOffset); IndexProxy diff --git a/llvm/lib/CAS/OnDiskGraphDB.cpp b/llvm/lib/CAS/OnDiskGraphDB.cpp index 99c9dd5f681c6..ab74d7117ef81 100644 --- a/llvm/lib/CAS/OnDiskGraphDB.cpp +++ b/llvm/lib/CAS/OnDiskGraphDB.cpp @@ -1102,10 +1102,11 @@ ObjectID OnDiskGraphDB::getExternalReference(const IndexProxy &I) { } std::optional -OnDiskGraphDB::getExistingReference(ArrayRef Digest) { +OnDiskGraphDB::getExistingReference(ArrayRef Digest, + bool CheckUpstream) { auto tryUpstream = [&](std::optional I) -> std::optional { - if (!UpstreamDB) + if (!CheckUpstream || !UpstreamDB) return std::nullopt; std::optional UpstreamID = UpstreamDB->getExistingReference(Digest); @@ -1138,6 +1139,15 @@ OnDiskGraphDB::getIndexProxyFromRef(InternalRef Ref) const { return getIndexProxyFromPointer(P); } +Expected +OnDiskGraphDB::getIndexProxyFromRefChecked(InternalRef Ref) const { + OnDiskHashMappedTrie::const_pointer P = + Index.recoverFromFileOffset(Ref.getFileOffset()); + if (LLVM_UNLIKELY(!P)) + return createStringError(make_error_code(std::errc::protocol_error), "corrupt internal reference"); + return getIndexProxyFromPointer(P); +} + ArrayRef OnDiskGraphDB::getDigest(InternalRef Ref) const { IndexProxy I = getIndexProxyFromRef(Ref); return I.Hash; @@ -1232,14 +1242,19 @@ OnDiskGraphDB::ObjectPresence OnDiskGraphDB::getObjectPresence(ObjectID ExternalRef, bool CheckUpstream) const { InternalRef Ref = getInternalRef(ExternalRef); - IndexProxy I = getIndexProxyFromRef(Ref); - TrieRecord::Data Object = I.Ref.load(); + Expected I = getIndexProxyFromRefChecked(Ref); + if (!I) { + // FIXME: this decision should be migrated to callers. + consumeError(I.takeError()); + return ObjectPresence::Missing; + } + TrieRecord::Data Object = I->Ref.load(); if (Object.SK != TrieRecord::StorageKind::Unknown) return ObjectPresence::InPrimaryDB; if (!CheckUpstream || !UpstreamDB) return ObjectPresence::Missing; std::optional UpstreamID = - UpstreamDB->getExistingReference(getDigest(I)); + UpstreamDB->getExistingReference(getDigest(*I)); return UpstreamID.has_value() ? ObjectPresence::OnlyInUpstreamDB : ObjectPresence::Missing; } diff --git a/llvm/lib/CAS/UnifiedOnDiskCache.cpp b/llvm/lib/CAS/UnifiedOnDiskCache.cpp index 64441ee8eb057..522e02b0ad188 100644 --- a/llvm/lib/CAS/UnifiedOnDiskCache.cpp +++ b/llvm/lib/CAS/UnifiedOnDiskCache.cpp @@ -165,11 +165,24 @@ Error UnifiedOnDiskCache::validateActionCache() { return createStringError( llvm::errc::illegal_byte_sequence, "bad record at 0x" + - utohexstr((unsigned)Offset.get(), /*LowerCase=*/true) + ": " + - Msg.str()); + utohexstr((unsigned)Offset.get(), /*LowerCase=*/true) + + " ref=0x" + utohexstr(ID.getOpaqueData(), /*LowerCase=*/true) + + ": " + Msg.str()); }; if (ID.getOpaqueData() == 0) return formatError("zero is not a valid ref"); + // Check containsObject first, because other API assumes a valid ObjectID. + if (!getGraphDB().containsObject(ID, /*CheckUpstream=*/false)) + return formatError("ref is not in cas index"); + auto Hash = getGraphDB().getDigest(ID); + auto Ref = getGraphDB().getExistingReference(Hash, /*CheckUpstream=*/false); + assert(Ref && "missing object passed containsObject check?"); + if (!Ref) + return formatError("ref is not in cas index after contains"); + if (*Ref != ID) + return formatError("ref does not match indexed offset " + + utohexstr(Ref->getOpaqueData(), /*LowerCase=*/true) + + " for hash " + toHex(Hash)); return Error::success(); }; if (Error E = PrimaryKVDB->validate(ValidateRef)) diff --git a/llvm/test/tools/llvm-cas/validation.test b/llvm/test/tools/llvm-cas/validation.test index 6faed5293098c..8a0d06447704d 100644 --- a/llvm/test/tools/llvm-cas/validation.test +++ b/llvm/test/tools/llvm-cas/validation.test @@ -26,6 +26,14 @@ RUN: --data - >%t/abc.casid RUN: llvm-cas --cas %t/ac --put-cache-key @%t/abc.casid @%t/empty.casid RUN: llvm-cas --cas %t/ac --validate + +# Check that validation fails if the objects referenced are missing. +RUN: mv %t/ac/v1.1/v9.index %t/tmp.v9.index +RUN: not llvm-cas --cas %t/ac --validate + +RUN: mv %t/tmp.v9.index %t/ac/v1.1/v9.index +RUN: llvm-cas --cas %t/ac --validate + # Note: records are 40 bytes (32 hash bytes + 8 byte value), so trim the last # allocated record, leaving it invalid. RUN: truncate -s -40 %t/ac/v1.1/v4.actions