Skip to content

Commit

Permalink
Merge pull request #2318 from meganz/hotfix/include_all_chunk_macs_on…
Browse files Browse the repository at this point in the history 
…_ultoken_arrival

SDK-1266. Hotfix: MAC verification failure
  • Loading branch information
@sergiohs84
sergiohs84 authored Oct 26, 2020
2 parents 4f13aa7 + 82fdcb3 commit 87bfccc
Show file tree
Hide file tree
Showing 8 changed files with 47 additions and 29 deletions.
2 changes: 1 addition & 1 deletion include/mega/transfer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ struct MEGA_API Transfer : public FileFingerprint
handletransfer_map::iterator faputcompletion_it;

// upload result
byte *ultoken;
unique_ptr<byte[]> ultoken;

// backlink to base
MegaClient* client;
Expand Down
2 changes: 1 addition & 1 deletion src/file.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ void File::completed(Transfer* t, LocalNode* l)
newnode->uploadhandle = t->uploadhandle;

// reference to uploaded file
memcpy(newnode->uploadtoken, t->ultoken, sizeof newnode->uploadtoken);
memcpy(newnode->uploadtoken, t->ultoken.get(), sizeof newnode->uploadtoken);

// file's crypto key
newnode->nodekey.assign((char*)t->filekey, FILENODEKEYLENGTH);
Expand Down
10 changes: 4 additions & 6 deletions src/megaclient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14619,7 +14619,7 @@ bool MegaClient::updateSyncRemoteLocation(const SyncConfig *config, Node *n, boo
if (!config)
{
LOG_err << "no config upon updateSyncRemotePath";
return API_ENOENT;
return false;
}
assert(syncConfigs);

Expand Down Expand Up @@ -15002,8 +15002,7 @@ bool MegaClient::startxfer(direction_t d, File* f, DBTableTransactionCommitter&
{
t->chunkmacs.clear();
t->progresscompleted = 0;
delete [] t->ultoken;
t->ultoken = NULL;
t->ultoken.reset();
t->pos = 0;
}
}
Expand Down Expand Up @@ -15041,8 +15040,7 @@ bool MegaClient::startxfer(direction_t d, File* f, DBTableTransactionCommitter&
t->tempurls.clear();
t->chunkmacs.clear();
t->progresscompleted = 0;
delete [] t->ultoken;
t->ultoken = NULL;
t->ultoken.reset();
t->pos = 0;
}
}
Expand Down Expand Up @@ -15218,7 +15216,7 @@ Node* MegaClient::nodebyfingerprint(LocalNode* localNode)
std::string localName =
localNode->localname.toName(*fsaccess,
localNode->sync->mFilesystemType);

// Only compare metamac if the node doesn't already exist.
node_vector::const_iterator remoteNode =
std::find_if(remoteNodes->begin(),
Expand Down
14 changes: 4 additions & 10 deletions src/transfer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,6 @@ Transfer::~Transfer()
client->asyncfopens--;
}

if (ultoken)
{
delete [] ultoken;
}

if (finished)
{
if (type == GET && !localfilename.empty())
Expand Down Expand Up @@ -180,7 +175,7 @@ bool Transfer::serialize(string *d)
{
hasUltoken = 2;
d->append((const char*)&hasUltoken, sizeof(char));
d->append((const char*)ultoken, NewNode::UPLOADTOKENLEN);
d->append((const char*)ultoken.get(), NewNode::UPLOADTOKENLEN);
}
else
{
Expand Down Expand Up @@ -311,8 +306,8 @@ Transfer *Transfer::unserialize(MegaClient *client, string *d, transfer_map* tra

if (hasUltoken)
{
t->ultoken = new byte[NewNode::UPLOADTOKENLEN]();
memcpy(t->ultoken, ptr, ll);
t->ultoken.reset(new byte[NewNode::UPLOADTOKENLEN]());
memcpy(t->ultoken.get(), ptr, ll);
ptr += ll;
}

Expand Down Expand Up @@ -500,8 +495,7 @@ void Transfer::failed(const Error& e, DBTableTransactionCommitter& committer, ds
{
chunkmacs.clear();
progresscompleted = 0;
delete [] ultoken;
ultoken = NULL;
ultoken.reset();
pos = 0;

if (slot && slot->fa && (slot->fa->mtime != mtime || slot->fa->size != size))
Expand Down
31 changes: 25 additions & 6 deletions src/transferslot.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -598,19 +598,19 @@ void TransferSlot::doio(MegaClient* client, DBTableTransactionCommitter& committ
LOG_debug << "Upload token received";
if (!transfer->ultoken)
{
transfer->ultoken = new byte[NewNode::UPLOADTOKENLEN]();
transfer->ultoken.reset(new byte[NewNode::UPLOADTOKENLEN]());
}

bool tokenOK = true;
if (reqs[i]->in.data()[NewNode::UPLOADTOKENLEN - 1] == 1)
{
LOG_debug << "New style upload token";
memcpy(transfer->ultoken, reqs[i]->in.data(), NewNode::UPLOADTOKENLEN);
memcpy(transfer->ultoken.get(), reqs[i]->in.data(), NewNode::UPLOADTOKENLEN);
}
else
{
LOG_debug << "Old style upload token: " << reqs[i]->in;
tokenOK = (Base64::atob(reqs[i]->in.data(), transfer->ultoken, NewNode::UPLOADTOKENLEN)
tokenOK = (Base64::atob(reqs[i]->in.data(), transfer->ultoken.get(), NewNode::UPLOADTOKENLEN)
== NewNode::OLDUPLOADTOKENLEN);
}

Expand All @@ -619,11 +619,27 @@ void TransferSlot::doio(MegaClient* client, DBTableTransactionCommitter& committ
errorcount = 0;
transfer->failcount = 0;

// any other connections that have not reported back yet, or we haven't processed yet,
// must have completed also - make sure to include their chunk MACs in the mac-of-macs
for (int j = connections; j--; )
{
if (j != i && reqs[j] &&
(reqs[j]->status == REQ_INFLIGHT
|| reqs[j]->status == REQ_SUCCESS
|| reqs[j]->status == REQ_FAILURE)) // could be a network error getting the result
{
LOG_debug << "Including chunk MACs from incomplete/unprocessed (at this end) connection " << j;
transfer->progresscompleted += reqs[j]->size;
transfer->chunkmacs.finishedUploadChunks(static_cast<HttpReqUL*>(reqs[j].get())->mChunkmacs);
}
}

transfer->chunkmacs.finishedUploadChunks(static_cast<HttpReqUL*>(reqs[i].get())->mChunkmacs);
transfer->progresscompleted += reqs[i]->size;
assert(transfer->progresscompleted == transfer->size);

updatecontiguousprogress();

transfer->progresscompleted += reqs[i]->size;
memcpy(transfer->filekey, transfer->transferkey.data(), sizeof transfer->transferkey);
((int64_t*)transfer->filekey)[2] = transfer->ctriv;
((int64_t*)transfer->filekey)[3] = macsmac(&transfer->chunkmacs);
Expand All @@ -643,8 +659,7 @@ void TransferSlot::doio(MegaClient* client, DBTableTransactionCommitter& committ
}
else
{
delete [] transfer->ultoken;
transfer->ultoken = NULL;
transfer->ultoken.reset();
}
}

Expand Down Expand Up @@ -1059,6 +1074,10 @@ void TransferSlot::doio(MegaClient* client, DBTableTransactionCommitter& committ
m_off_t pos = posrange.first;
unsigned size = (unsigned)(posrange.second - pos);

// No need to keep recopying already processed macs from prior uploads on this req[i]
// For uploads, these are always on chunk boundaries so no need to worry about partials.
static_cast<HttpReqUL*>(reqs[i].get())->mChunkmacs.clear();

if (fa->asyncavailable())
{
if (asyncIO[i])
Expand Down
1 change: 1 addition & 0 deletions src/utils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,7 @@ int64_t chunkmac_map::macsmac(SymmCipher *cipher)
m[0] ^= m[1];
m[1] = m[2] ^ m[3];

// LOG_debug << "macsmac final: " << Base64Str<sizeof int64_t>(mac);
return MemAccess::get<int64_t>((const char*)mac);
}

Expand Down
6 changes: 6 additions & 0 deletions tests/integration/SdkTest_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -784,6 +784,8 @@ void SdkTest::createFile(string filename, bool largeFile)
if (largeFile)
limit = 1000000 + rand() % 1000000;

//limit = 5494065 / 5;

for (int i = 0; i < limit; i++)
{
fprintf(fp, "test ");
Expand Down Expand Up @@ -4086,6 +4088,7 @@ TEST_F(SdkTest, SdkTestOverquotaNonCloudraid)
LOG_info << "___TEST SdkTestOverquotaNonCloudraid";
getAccountsForTest(2);

//for (int i = 0; i < 1000; ++i) {
ASSERT_TRUE(DebugTestHook::resetForTests()) << "SDK test hooks are not enabled in release mode";

// make a file to download, and upload so we can pull it down
Expand Down Expand Up @@ -4142,6 +4145,9 @@ TEST_F(SdkTest, SdkTestOverquotaNonCloudraid)
ASSERT_LT(DebugTestHook::countdownToOverquota, originalcount); // there should have been more http activity after the wait

ASSERT_TRUE(DebugTestHook::resetForTests()) << "SDK test hooks are not enabled in release mode";

//cout << "Passed round " << i << endl; }

}
#endif

Expand Down
10 changes: 5 additions & 5 deletions tests/unit/Transfer_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ void checkTransfers(const mega::Transfer& exp, const mega::Transfer& act)
exp.transferkey.data() + mega::SymmCipher::KEYLENGTH,
act.transferkey.data()));
ASSERT_EQ(exp.lastaccesstime, act.lastaccesstime);
ASSERT_TRUE(std::equal(exp.ultoken, exp.ultoken + mega::NewNode::UPLOADTOKENLEN, act.ultoken));
ASSERT_TRUE(std::equal(exp.ultoken.get(), exp.ultoken.get() + mega::NewNode::UPLOADTOKENLEN, act.ultoken.get()));
ASSERT_EQ(exp.tempurls, act.tempurls);
ASSERT_EQ(exp.state, act.state);
ASSERT_EQ(exp.priority, act.priority);
Expand All @@ -64,8 +64,8 @@ TEST(Transfer, serialize_unserialize)
tf.transferkey.data() + mega::SymmCipher::KEYLENGTH,
'Y');
tf.lastaccesstime = 3;
tf.ultoken = new mega::byte[mega::NewNode::UPLOADTOKENLEN];
std::fill(tf.ultoken, tf.ultoken + mega::NewNode::UPLOADTOKENLEN, 'Z');
tf.ultoken.reset(new mega::byte[mega::NewNode::UPLOADTOKENLEN]);
std::fill(tf.ultoken.get(), tf.ultoken.get() + mega::NewNode::UPLOADTOKENLEN, 'Z');
tf.tempurls = {
"http://bar1.com",
"http://bar2.com",
Expand Down Expand Up @@ -102,8 +102,8 @@ TEST(Transfer, unserialize_32bit)
tf.transferkey.data() + mega::SymmCipher::KEYLENGTH,
'Y');
tf.lastaccesstime = 3;
tf.ultoken = new mega::byte[mega::NewNode::UPLOADTOKENLEN];
std::fill(tf.ultoken, tf.ultoken + mega::NewNode::UPLOADTOKENLEN, 'Z');
tf.ultoken.reset(new mega::byte[mega::NewNode::UPLOADTOKENLEN]);
std::fill(tf.ultoken.get(), tf.ultoken.get() + mega::NewNode::UPLOADTOKENLEN, 'Z');
tf.tempurls = {
"http://bar1.com",
"http://bar2.com",
Expand Down

0 comments on commit 87bfccc

Please sign in to comment.