From b393947cd596ce461b887b6f02fc54d31ec94c63 Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Thu, 29 May 2014 12:09:24 -0400 Subject: [PATCH] Fix dupe callbacks in VaultAddChildNode This code called back into the python vault operation thingy, which deletes itself. So, anytime we got dupe CBs, there was a use-after-free error. Nasty crashes. I tightened up the CB management, threw away some weird faux-management mess, and squashed some potential silent bugs. --- .../PubUtilLib/plVault/plVaultClientApi.cpp | 137 +++++++++--------- 1 file changed, 67 insertions(+), 70 deletions(-) diff --git a/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.cpp b/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.cpp index 3a4ec3ec..51705050 100644 --- a/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.cpp +++ b/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.cpp @@ -246,18 +246,27 @@ struct VaultAgeInitTrans { ); }; -struct AddChildNodeFetchTrans { +class AddChildNodeTrans { FVaultAddChildNodeCallback callback; void * cbParam; ENetError result; std::atomic opCount; - AddChildNodeFetchTrans() - : callback(nil), cbParam(nil), result(kNetSuccess), opCount(0) { } + void CompleteOp(); - AddChildNodeFetchTrans(FVaultAddChildNodeCallback _callback, void * _param) +public: + AddChildNodeTrans() + : callback(nullptr), cbParam(nullptr), result(kNetSuccess), opCount(0) { } + + AddChildNodeTrans(FVaultAddChildNodeCallback _callback, void * _param) : callback(_callback), cbParam(_param), result(kNetSuccess), opCount(0) { } + void AddOp() { ++opCount; } + + static void VaultNodeAdded ( + ENetError result, + void * param + ); static void VaultNodeFetched ( ENetError result, void * param, @@ -994,18 +1003,38 @@ void VaultAgeInitTrans::AgeInitCallback ( /***************************************************************************** * -* AddChildNodeFetchTrans +* AddChildNodeTrans * ***/ //============================================================================ -void AddChildNodeFetchTrans::VaultNodeRefsFetched ( +void AddChildNodeTrans::CompleteOp() { + if ((--opCount) == 0) { + if (callback) + callback(result, cbParam); + delete this; // commit hara-kiri + } +} + +//============================================================================ +void AddChildNodeTrans::VaultNodeAdded( + ENetError result, + void* param +) { + AddChildNodeTrans* trans = static_cast(param); + if (IS_NET_ERROR(result)) + trans->result = result; + trans->CompleteOp(); +} + +//============================================================================ +void AddChildNodeTrans::VaultNodeRefsFetched ( ENetError result, void * param, NetVaultNodeRef * refs, unsigned refCount ) { - AddChildNodeFetchTrans * trans = (AddChildNodeFetchTrans *)param; + AddChildNodeTrans * trans = (AddChildNodeTrans *)param; if (IS_NET_ERROR(result)) { trans->result = result; @@ -1015,45 +1044,27 @@ void AddChildNodeFetchTrans::VaultNodeRefsFetched ( FetchNodesFromRefs( refs, refCount, - AddChildNodeFetchTrans::VaultNodeFetched, + AddChildNodeTrans::VaultNodeFetched, param, &incFetchCount ); trans->opCount += incFetchCount; } - - // Make the callback now if there are no nodes to fetch, or if error - if (!(--trans->opCount)) { - if (trans->callback) - trans->callback( - trans->result, - trans->cbParam - ); - delete trans; - } + trans->CompleteOp(); } //============================================================================ -void AddChildNodeFetchTrans::VaultNodeFetched ( +void AddChildNodeTrans::VaultNodeFetched ( ENetError result, void * param, NetVaultNode * node ) { ::VaultNodeFetched(result, param, node); - - AddChildNodeFetchTrans * trans = (AddChildNodeFetchTrans *)param; - + + AddChildNodeTrans * trans = (AddChildNodeTrans *)param; if (IS_NET_ERROR(result)) trans->result = result; - - if (!(--trans->opCount)) { - if (trans->callback) - trans->callback( - trans->result, - trans->cbParam - ); - delete trans; - } + trans->CompleteOp(); } @@ -1737,9 +1748,6 @@ void VaultAddChildNode ( FVaultAddChildNodeCallback callback, void * param ) { - // Make sure we only do the callback once - bool madeCallback = false; - // Too much of the client relies on the assumption that the node will be immediately // associated with its parent. THIS SUCKS, because there's no way to guarantee the // association won't be circular (the db checks this in a comprehensive way). @@ -1748,14 +1756,14 @@ void VaultAddChildNode ( // This directly affects: New clothing items added to the avatar outfit folder, // new chronicle entries in some ages, and I'm sure several other situations. + AddChildNodeTrans * trans = nullptr; if (RelVaultNodeLink * parentLink = s_nodes.Find(parentId)) { RelVaultNodeLink * childLink = s_nodes.Find(childId); if (!childLink) { childLink = new RelVaultNodeLink(false, ownerId, childId, new RelVaultNode); childLink->node->SetNodeId_NoDirty(childId); s_nodes.Add(childLink); - } - else if (ownerId) { + } else if (ownerId) { childLink->ownerId = ownerId; } @@ -1770,14 +1778,17 @@ void VaultAddChildNode ( // callback now with error code if (callback) callback(kNetErrCircularReference, param); - } - else if (childLink->node->IsParentOf(parentId, 255)) { + // I don't really care what Eric thinks, if we believe it's a circular reference, it's evil + // to report a failure, then send it to the server anyway! That's just a bug WAITING + // to be uncovered, and I don't want to deal with it! + return; + } else if (childLink->node->IsParentOf(parentId, 255)) { LogMsg(kLogDebug, L"Node relationship would be circular: p:%u, c:%u", parentId, childId); // callback now with error code if (callback) callback(kNetErrCircularReference, param); - } - else { + return; + } else { NetVaultNodeRef refs[] = { { parentId, childId, ownerId } }; @@ -1786,66 +1797,52 @@ void VaultAddChildNode ( ARRAY(unsigned) existingNodeIds; BuildNodeTree(refs, arrsize(refs), &newNodeIds, &existingNodeIds); - + + trans = new AddChildNodeTrans(callback, param); if (!childLink->node->GetNodeType() || !parentLink->node->GetNodeType()) { // One or more nodes need to be fetched before the callback is made - AddChildNodeFetchTrans * trans = new AddChildNodeFetchTrans(callback, param); if (!childLink->node->GetNodeType()) { - ++trans->opCount; + trans->AddOp(); NetCliAuthVaultNodeFetch( childId, - AddChildNodeFetchTrans::VaultNodeFetched, + AddChildNodeTrans::VaultNodeFetched, trans ); - ++trans->opCount; + trans->AddOp(); NetCliAuthVaultFetchNodeRefs( childId, - AddChildNodeFetchTrans::VaultNodeRefsFetched, + AddChildNodeTrans::VaultNodeRefsFetched, trans ); } if (!parentLink->node->GetNodeType()) { - ++trans->opCount; + trans->AddOp(); NetCliAuthVaultNodeFetch( parentId, - AddChildNodeFetchTrans::VaultNodeFetched, + AddChildNodeTrans::VaultNodeFetched, trans ); - ++trans->opCount; + trans->AddOp(); NetCliAuthVaultFetchNodeRefs( parentId, - AddChildNodeFetchTrans::VaultNodeRefsFetched, + AddChildNodeTrans::VaultNodeRefsFetched, trans ); } } - else { - // We have both nodes already, so make the callback now. - if (callback) { - callback(kNetSuccess, param); - madeCallback = true; - } - } - } - } - else { - // Parent doesn't exist locally (and we may not want it to), just make the callback now. - if (callback) { - callback(kNetSuccess, param); - madeCallback = true; } } - // Send it on up to the vault. The db server filters out duplicate and - // circular node relationships. We send the request up even if we think - // the relationship would be circular since the db does a universal - // check and is the only real authority in this matter. + // Send it off to the vault. Note that we're reusing the old fetch trans's opCount junk + // to ensure that we only ever callback once. May Jesus be with us all. + if (trans) + trans->AddOp(); NetCliAuthVaultNodeAdd( parentId, childId, ownerId, - madeCallback ? nil : callback, - madeCallback ? nil : param + trans ? AddChildNodeTrans::VaultNodeAdded : callback, + trans ? trans : param ); }