Skip to content

35 bug getasset returns copy of asset ident not pointer #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tadeas-hejnic wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

35 bug getasset returns copy of asset ident not pointer #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
50fdd27
Avoid copy in getAsset: getAsset ident returns a pointer instead of copy
tadeas-hejnic Jan 30, 2025
e2f1f9d
Avoid copy in getAsset: implementation of getAsset to return a pointe…
tadeas-hejnic Jan 30, 2025
60cabb2
Avoid copy in getAsset: small adjustmenst in resolver caused by chang…
tadeas-hejnic Jan 30, 2025
1a81fdd
Avoid copy in getAsset: implementation of getAsset to return a pointe…
tadeas-hejnic Jan 30, 2025
79435be
Small adjustment in _Resolve, + explaining comment
tadeas-hejnic Feb 7, 2025
c87c4c0
resolve merge conflicts
tadeas-hejnic Apr 13, 2026
bf598ca
Merge branch 'develop' into 35-bug-getasset-returns-copy-of-asset-ide…
tadeas-hejnic Apr 13, 2026
8a97620
assetIdent -> AssetIdentifier
tadeas-hejnic Apr 13, 2026
c5eb7b4
assetIdent -> AssetIdentifier n.2
tadeas-hejnic Apr 13, 2026
8b2f902
removing reference where its not needed
tadeas-hejnic Apr 13, 2026
7af7acf
fix incorrect variable name
tadeas-hejnic Apr 13, 2026
bf6488c
fix incorrect type name
tadeas-hejnic Apr 13, 2026
81122a7
fix build issues
tadeas-hejnic Apr 13, 2026
d775dab
fix build issue
tadeas-hejnic Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 27 additions & 25 deletions src/AyonUsdResolver/cache/resolverContextCache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ PinningFileHandler::PinningFileHandler(const std::string &pinningFilePath,
* @param resolveKey UsdAssetIdent
* @return populated AssetIdentifier if key was found in pinning file. Empty AssetIdentifier if key was not found
*/
AssetIdentifier
AssetIdentifier*
PinningFileHandler::getAssetData(const std::string &resolveKey) {
AssetIdentifier assetEntry;
AssetIdentifier* assetEntry = nullptr;

std::string pinnedAssetPath;
try {
Expand All @@ -76,9 +76,11 @@ PinningFileHandler::getAssetData(const std::string &resolveKey) {
return assetEntry;
}

assetEntry = new AssetIdentifier();

if (!pinnedAssetPath.empty()) {
assetEntry.setAssetIdentifier(resolveKey);
assetEntry.setResolvedAssetPath(pinnedAssetPath);
assetEntry->setAssetIdentifier(resolveKey);
assetEntry->setResolvedAssetPath(pinnedAssetPath);
}

return assetEntry;
Comment on lines 67 to 86
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function now returns a raw pointer allocated with new AssetIdentifier() but there is no corresponding ownership contract or delete, so every successful lookup leaks. Also, when the key is missing you return nullptr, which forces all callers to handle null (and resolver.cpp currently doesn't). Prefer returning by value / std::optional<AssetIdentifier> / std::unique_ptr<AssetIdentifier> (with clear ownership), or return a const AssetIdentifier&/pointer to stable storage managed by the handler.

Copilot uses AI. Check for mistakes.
Expand Down Expand Up @@ -163,32 +165,31 @@ ResolverContextCache::migratePreCacheIntoAyonCache() {
m_PreCache.clear();
};

AssetIdentifier
AssetIdentifier*
ResolverContextCache::getAsset(const std::string &assetIdentifier,
const CacheName selectedCache,
const bool isAyonPath) {
TF_DEBUG(AYONUSDRESOLVER_RESOLVER_CONTEXT).Msg("ResolverContextCache::getAsset: (%s) \n", assetIdentifier.c_str());

AssetIdentifier asset;

if (assetIdentifier.empty()) {
return asset;
return new AssetIdentifier();
}
if (m_staticCache) {
return m_pinningFileHandler->getAssetData(assetIdentifier);
}
Comment on lines 174 to 179
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAsset allocates with new AssetIdentifier() on the empty-identifier path. This leaks unless every caller deletes the returned pointer, which is not documented or implemented anywhere in the repo. If you want to avoid copies, prefer returning std::optional<std::reference_wrapper<const AssetIdentifier>>, const AssetIdentifier* to cache-owned storage, or a smart pointer that clearly expresses ownership.

Copilot uses AI. Check for mistakes.

std::unordered_set<AssetIdentifier, AssetIdentifierHash>::iterator hit;

AssetIdentifier* asset = nullptr;

std::shared_lock<std::shared_mutex> preCacheSharedLock(m_PreCacheSharedMutex);
hit = m_PreCache.find(assetIdentifier);
if (hit != m_PreCache.end()) {
asset = *hit;
asset = const_cast<AssetIdentifier*>(&(*hit)); // get the pointer without making a copy of the object
preCacheSharedLock.unlock();

TF_DEBUG(AYONUSDRESOLVER_RESOLVER_CONTEXT)
.Msg("ResolverContextCache::getAsset: PreCache Hit on (%s) with (%s) \n",
asset.getAssetIdentifier().c_str(), asset.getResolvedAssetPath().GetPathString().c_str());
asset->getAssetIdentifier().c_str(), asset->getResolvedAssetPath().GetPathString().c_str());
return asset;
Comment on lines 184 to 193
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning a non-const AssetIdentifier* to an element inside an std::unordered_set via const_cast is unsafe: callers could mutate fields that participate in hashing/equality (m_assetIdentifier), breaking container invariants and causing undefined behavior. If you need to return a pointer, make it const AssetIdentifier* (and remove the const_cast), or change the cache storage to something designed for stable, mutable values (e.g., unordered_map<std::string, AssetIdentifier>).

Copilot uses AI. Check for mistakes.
}
preCacheSharedLock.unlock();
Expand All @@ -199,7 +200,7 @@ ResolverContextCache::getAsset(const std::string &assetIdentifier,
std::shared_lock<std::shared_mutex> ayonCacheSharedLock(m_AyonCacheSharedMutex);
hit = m_AyonCache.find(assetIdentifier);
if (hit != m_AyonCache.end()) {
asset = *hit;
asset = const_cast<AssetIdentifier*>(&(*hit)); // get the pointer without making a copy of the object
TF_DEBUG(AYONUSDRESOLVER_RESOLVER_CONTEXT).Msg("ResolverContextCache::getAsset: AyonCache Hit \n");
}

Expand All @@ -212,7 +213,7 @@ ResolverContextCache::getAsset(const std::string &assetIdentifier,
std::shared_lock<std::shared_mutex> CommonCacheSharedLock(m_CommonCacheSharedMutex);
hit = m_CommonCache.find(assetIdentifier);
if (hit != m_CommonCache.end()) {
asset = *hit;
asset = const_cast<AssetIdentifier*>(&(*hit)); // get the pointer without making a copy of the object
TF_DEBUG(AYONUSDRESOLVER_RESOLVER_CONTEXT)
.Msg("ResolverContextCache::getAsset: CommonCache Hit \n");
}
Expand All @@ -221,38 +222,39 @@ ResolverContextCache::getAsset(const std::string &assetIdentifier,
break;
}
}
if (!asset.isEmpty()) {
if (asset != nullptr) {
TF_DEBUG(AYONUSDRESOLVER_RESOLVER_CONTEXT)
.Msg("ResolverContextCache::getAsset: Cache Hit with (%s) with (%s) \n", asset.getAssetIdentifier().c_str(),
asset.getResolvedAssetPath().GetPathString().c_str());
.Msg("ResolverContextCache::getAsset: Cache Hit with (%s) with (%s) \n", asset->getAssetIdentifier().c_str(),
asset->getResolvedAssetPath().GetPathString().c_str());
return asset;
}

asset = new AssetIdentifier();

TF_DEBUG(AYONUSDRESOLVER_RESOLVER_CONTEXT).Msg("ResolverContextCache::getAsset: No Cache Hit \n");
if (isAyonPath) {
std::pair<std::string, std::string> resolvedAsset = m_ayon->get()->resolvePath(assetIdentifier);

asset.setAssetIdentifier(std::move(resolvedAsset.first));
asset.setResolvedAssetPath(std::move(resolvedAsset.second));
asset->setAssetIdentifier(std::move(resolvedAsset.first));
asset->setResolvedAssetPath(std::move(resolvedAsset.second));

TF_DEBUG(AYONUSDRESOLVER_RESOLVER_CONTEXT).Msg("ResolverContextCache::getAsset: called ayon.resolvePath() \n");
this->insert(asset);
this->insert(*asset);
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the AYON resolve branch you call this->insert(*asset), but insert() moves from its argument (m_PreCache.insert(std::move(sourceAssetIdent))). That means the heap object *asset is left in a moved-from/empty state, yet you still return the pointer asset at the end of the function. This will cause callers to observe an empty/invalid AssetIdentifier despite having inserted the resolved entry into the cache. You likely want to either (1) insert a copy and keep returning the same object, or (2) insert and then return a pointer/reference to the cached element (not the moved-from temporary).

Suggested change
this->insert(*asset);
AssetIdentifier cachedAsset(*asset);
this->insert(cachedAsset);

Copilot uses AI. Check for mistakes.
}
else {
if (_IsRelativePath(assetIdentifier)) {
asset.setResolvedAssetPath(_ResolveAnchored(ArchGetCwd(), assetIdentifier));
asset->setResolvedAssetPath(_ResolveAnchored(ArchGetCwd(), assetIdentifier));
}
else {
asset.setResolvedAssetPath(ArResolvedPath(TfNormPath(TfAbsPath(assetIdentifier))));
asset->setResolvedAssetPath(ArResolvedPath(TfNormPath(TfAbsPath(assetIdentifier))));
}
if (!asset.getResolvedAssetPath().empty()) {
asset.setAssetIdentifier(assetIdentifier);
if (!asset->getResolvedAssetPath().empty()) {
asset->setAssetIdentifier(assetIdentifier);

std::shared_lock<std::shared_mutex> CommonCacheSharedLock(m_CommonCacheSharedMutex);

TF_DEBUG(AYONUSDRESOLVER_RESOLVER_CONTEXT)
.Msg("ResolverContextCache::getAsset: insert into CommonCache \n");
m_CommonCache.insert(asset);
m_CommonCache.insert(*asset);
}
Comment on lines 253 to 258
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This inserts into m_CommonCache while holding only a std::shared_lock on m_CommonCacheSharedMutex. unordered_set::insert is a write operation and requires exclusive locking; doing it under a shared lock is a data race in multi-threaded use. Switch this to std::unique_lock (or reuse existing write-locking patterns used elsewhere in this file).

Copilot uses AI. Check for mistakes.
}

Expand Down
4 changes: 2 additions & 2 deletions src/AyonUsdResolver/cache/resolverContextCache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class PinningFileHandler {
const std::unordered_map<std::string, std::string> &rootReplaceData);
~PinningFileHandler() = default;

AssetIdentifier getAssetData(const std::string &resolveKey);
AssetIdentifier* getAssetData(const std::string &resolveKey);

private:
std::filesystem::path m_pinningFilePath;
Expand Down Expand Up @@ -62,7 +62,7 @@ class ResolverContextCache {
* @param isAyonPath Whether this is an AYON URI
* @return AssetIdentifier with resolved path
*/
AssetIdentifier getAsset(const std::string &assetIdentifier, const CacheName selectedCache, const bool isAyonPath);
AssetIdentifier* getAsset(const std::string &assetIdentifier, const CacheName selectedCache, const bool isAyonPath);

Comment on lines 63 to 66
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header change to return raw pointers (AssetIdentifier*) introduces unclear lifetime/ownership for API consumers and makes it easy to leak or dereference a dangling/null pointer. If the goal is to avoid copying, prefer returning const AssetIdentifier*/const AssetIdentifier& tied to cache lifetime, or a smart pointer/optional that makes ownership explicit.

Copilot uses AI. Check for mistakes.
/**
* @brief Set up the cache from a pinning file
Expand Down
4 changes: 2 additions & 2 deletions src/AyonUsdResolver/resolver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ AyonUsdResolver::_Resolve(const std::string &assetPath) const {
continue;
}

AssetIdentifier asset;
AssetIdentifier* asset = nullptr;
std::string cleanAssetPath = *pathToResolve;
RES_FUNCS_REMOVE_SDF_ARGS(cleanAssetPath);
asset = resolverCache->getAsset(cleanAssetPath, CacheName::AYONCACHE, true);
Expand All @@ -161,7 +161,7 @@ AyonUsdResolver::_Resolve(const std::string &assetPath) const {
if (pos != std::string::npos) {
sdfArgs = pathToResolve->substr(pos + cleanAssetPath.length());
}
std::string resolvedPathStr = asset.getResolvedAssetPath().GetPathString() + sdfArgs;
std::string resolvedPathStr = asset->getResolvedAssetPath().GetPathString() + sdfArgs;
ArResolvedPath resolvedPath(resolvedPathStr);
Comment on lines 154 to 165
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolverCache->getAsset() can return nullptr (e.g., PinningFileHandler::getAssetData returns nullptr when resolveKey is not present). This code unconditionally dereferences asset (asset->getResolvedAssetPath()), which will crash for missing pinning entries or other failure paths. Consider making getAsset() never return null (e.g., return a reference/optional), or add a null check here and fall back to trying the next context / returning the original path.

Copilot uses AI. Check for mistakes.

if (resolvedPath) {
Expand Down